Github user tzulitai commented on the issue:
https://github.com/apache/flink/pull/4473
No problem! And really sorry for the wait.
I'm waiting for a Travis run before merging:
https://travis-ci.org/tzulitai/flink/builds/272758087?utm_source=github_status_medium=notification
---
Github user bowenli86 commented on the issue:
https://github.com/apache/flink/pull/4473
@tzulitai Thank you, Gordon!
---
Github user tzulitai commented on the issue:
https://github.com/apache/flink/pull/4473
Hi @bowenli86, sorry about this. I had the commit ready to merge but was
waiting for another test PR to be merged first. Merging this now!
---
Github user bowenli86 commented on the issue:
https://github.com/apache/flink/pull/4473
@tzulitai or other Flink committers, can you please merge this so I can
submit more PRs depend on this? Thanks!
---
Github user bowenli86 commented on the issue:
https://github.com/apache/flink/pull/4473
can anyone from data artisan take a look at this PR please?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not
Github user bowenli86 commented on the issue:
https://github.com/apache/flink/pull/4473
@tzulitai any more feedbacks? We have a ticket on my company for this task,
and I'd like to mark it as finished if possible :)
---
If your project is set up for it, you can reply to this email
Github user bowenli86 commented on the issue:
https://github.com/apache/flink/pull/4473
@tzulitai done!
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or
Github user tzulitai commented on the issue:
https://github.com/apache/flink/pull/4473
LGTM! I'll rebase this on #4537, and will merge as soon as Travis gives
green.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If
Github user tzulitai commented on the issue:
https://github.com/apache/flink/pull/4473
@bowenli86 I made some comments on the serializability of the
`KinesisProducerConfiguration`. That isn't serializable. Perhaps that is what
is causing the failure for you. I'm not really sure why
Github user bowenli86 commented on the issue:
https://github.com/apache/flink/pull/4473
@tzulitai it's failing on
```
The implementation of the RichSinkFunction is not serializable. The object
probably contains or references non serializable fields.
Github user tzulitai commented on the issue:
https://github.com/apache/flink/pull/4473
@bowenli86 what is it failing on? AFAIK, the only serialization changes in
1.3.2 compared to 1.3.0 are serialization of some specific `TypeSerializer`s.
---
If your project is set up for it, you
Github user bowenli86 commented on the issue:
https://github.com/apache/flink/pull/4473
@tzulitai Thanks Gordan for the quick response! You are right, I set the
schema in unit test to be static and it passes.
For my Flink job. When I build it against Flink 1.3.0, it can run
Github user tzulitai commented on the issue:
https://github.com/apache/flink/pull/4473
@bowenli86 on the other hand, in the constructors of
`FlinkKinesisProducer`, we probably should add eager safe checks on the
serializability of the provided `KinesisSerializationSchema` and have
Github user tzulitai commented on the issue:
https://github.com/apache/flink/pull/4473
@bowenli86 I think the test you posted above is failing because the
provided `KinesisSerializationSchema` is not serializable. It is an anonymous
class, which would contain a reference to the
Github user bowenli86 commented on the issue:
https://github.com/apache/flink/pull/4473
hmmm... I was deploying our Flink job with this change. The Flink job
failed to start, and log reports:
```
The implementation of the RichSinkFunction is not serializable. The object
Github user bowenli86 commented on the issue:
https://github.com/apache/flink/pull/4473
@tzulitai Let me know what you think about this PR now :)
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not
Github user tzulitai commented on the issue:
https://github.com/apache/flink/pull/4473
Thanks! Overall I think the changes are good.
I've left some minor cosmetic-related comments, and some more involved
comments on how we perform the deprecation.
---
If your project is set up
Github user bowenli86 commented on the issue:
https://github.com/apache/flink/pull/4473
@tzulitai @aljoscha Let me know if this PR looks good. Thanks!
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does
Github user tzulitai commented on the issue:
https://github.com/apache/flink/pull/4473
@bowenli86 no, they're provided via a `Properties` when creating a
`FlinkKinesisProducer`
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as
Github user bowenli86 commented on the issue:
https://github.com/apache/flink/pull/4473
Trying to understand the keys. Are keys like
"aws.producer.collectionMaxCount" specified in flink-conf.yaml?
---
If your project is set up for it, you can reply to this email and have your
reply
Github user tzulitai commented on the issue:
https://github.com/apache/flink/pull/4473
+1 to option 3. Looks like we all agree on that :) @bowenli86 please feel
free to continue with that.
---
If your project is set up for it, you can reply to this email and have your
reply appear
Github user aljoscha commented on the issue:
https://github.com/apache/flink/pull/4473
I think option 3) is the way to go. For backwards compatibility we have to
manually convert those keys that we already support (via
`ProducerConfigConstants`) but luckily they're not that many.
Github user bowenli86 commented on the issue:
https://github.com/apache/flink/pull/4473
I'm in favor of the 3rd option. In that way, we don't need to 1) duplicate
all KPL keys and documentation to flink-kinesis-connector and 2) always keep an
eye on KPL configs when upgrading its
Github user tzulitai commented on the issue:
https://github.com/apache/flink/pull/4473
Also cc'ing @aljoscha to this PR, since we also had a bit of offline
discussion on this configuration gap issue.
---
If your project is set up for it, you can reply to this email and have your
Github user tzulitai commented on the issue:
https://github.com/apache/flink/pull/4473
@bowenli86 the third option actually strikes me as an overall good
solution, I didn't know that was possible.
I think it would make sense to deprecate the previous configuration keys in
Github user bowenli86 commented on the issue:
https://github.com/apache/flink/pull/4473
Hi @tzulitai ,
Let's reach a consensus before I do any more works.
1) I didn't add all KPL's configs in this PR. I only added some
configs that I think might be useful to
Github user tzulitai commented on the issue:
https://github.com/apache/flink/pull/4473
One other thing:
Also need to add validation for these new configurations.
That should be placed in `KinesisConfigUtil.validateProducerConfigs`
---
If your project is set up for it, you can
Github user bowenli86 commented on the issue:
https://github.com/apache/flink/pull/4473
@tzulitai Hi Gordon, shall we add more explanation of these keys in Flink
docs, or do you think the java doc is sufficient?
---
If your project is set up for it, you can reply to this email and
28 matches
Mail list logo
