Wow, that is one thorough FLIP! I didn't fully go into all the technical
details but I think the general direction of this is good. If no one
objects I'd say we can proceed to voting and figure out the technical
details during implementation/review (if any remain unclear).
Best,
Aljoscha
On 29.06.20 16:27, Cranmer, Danny wrote:
Hey Gordon,
Thank-you for you review and feedback.
I agree with your suggestion for the contribution plan. I have updated the FLIP
to include the additional step with the FanOutKinesisProxy/AWS SDK 2.x
dependency. I have also added another precursor step to generally improve test
coverage. I have been playing around in the connector code and discovered that
restarting consumption from an aggregated record is not covered by
unit/integration tests. I have written some additional tests with simulated
Kinesis behaviour, pushing these tests in advance would increase confidence in
the next contribution step.
Regarding the consumer de-/registration. I had not considered making it
optional via configuration as you propose. I agree with your observation on the
additional configuration complexity and it would also expose internal
implementation details to the user. That being said, if a user application has
a very high parallelism they could quite easily exceed the ListStreamConsumers
quota (5 TPS [1]), and increase the application start-up time substantially
with back-off delays. The client could conditionally (based on config) register
the stream consumer and add the ConsumerARN(s) to the consumer properties,
eliminating the required parallel calls to ListStreamConsumers by the Flink
tasks. I will update the FLIP to include this change, and reply to this thread
once it is done.
[1]
https://docs.aws.amazon.com/kinesis/latest/APIReference/API_ListStreamConsumers.html
Thanks,
Danny,
On 29/06/2020, 05:19, "Tzu-Li (Gordon) Tai" <tzuli...@apache.org> wrote:
CAUTION: This email originated from outside of the organization. Do not
click links or open attachments unless you can confirm the sender and know the
content is safe.
Also, if it wasn't clear, I'll be happy to provide committer support on
reviewing and merging this FLIP, if it gets approved :)
On Mon, Jun 29, 2020 at 12:12 PM Tzu-Li (Gordon) Tai <tzuli...@apache.org>
wrote:
> Hi Cranmer,
>
> Thank you for proposing the feature and starting the discussion thread.
> This is really great work!
>
> Overall, +1 to adding EFO support to the Kinesis connector.
> I can see that having a dedicated throughput quota for each consuming
> Flink application is definitely a requirement for AWS users.
> In the past, we worked around this by using adaptive polling to avoid
> exceeding the quotas with multiple consumers, this would probably go away
> with this implemented.
>
> There are a few things I like about the current proposal:
> - EFO is an opt-in feature for now. Once we decide to reimplement the
> Kinesis connector on top of the new source interface (FLIP-27), we can
> probably consider enabling EFO by default to match the defaults of the
> higher-level KCL library.
> - From the design, it seems like the changes can indeed be fairly
> consolidated in the Kinesis connector. The change should be fairly safe
as
> well, since we're essentially abstracting record publishing concerns
only,
> which is transparent to the exactly-once semantics / watermark
components.
>
> Concerning competing stream consumer de-/registration:
> this would most likely go away with the new source interface, where this
> can be done on the source split enumerator.
> I'm personally okay with the proposed strategy of competing with backoff.
> As food for thought, have you considered an opt-in / opt-out, where the
> user knows that the client has access to KDS, and can choose to register
> once only on the client side instead of competing in TMs?
> I'm not sure if this is worth the extra configuration complexity though.
>
> For the concrete next steps for implementation after this FLIP has
passed:
> From the FLIP I can conclude that implementation wise, this would come in
> a few steps -
> 1. Abstract away record publishing behind a new interface. Initially we
> only have one implementation (the current polling mechanism).
> 2. Add in AWS SDK 2.x dependency, with a new FanOutKinesisProxy
> implementation.
> 3. Add FanOutRecordPublisher to finalize EFO support.
>
> I think step 2. wasn't explicitly mentioned in the FLIP, but I strongly
> suggest to consolidate that step as a single PR, as we would need to do a
> license check for dependency changes and it would be nice to move forward
> with that with at least interference of code changes as possible.
>
> Pushing this FLIP forward for approval:
> Since this FLIP is a fairly consolidated change, we should be safe to
> proceed with a vote soon.
> That usually happens in a separate vote thread, linking to this
discussion
> thread.
>
> cc'ing Thomas Weise as well, as he has also worked substantially on the
> Kinesis connector in the past.
>
> Cheers,
> Gordon
>
> On Tue, Jun 23, 2020 at 6:02 PM hey_wxl <xiaolong.w...@smartnews.com>
> wrote:
>
>> Hi, Cranmer.
>>
>> I'm Roland Wang. I've read the FLIP you wrote, and agree with your
>> design.
>> Recently, I'm working on this feature too, and have made some
progress:
>>
>> 1. I add two methods: getOrRegisterConsumer & subscribeToShard on
>> KinesisProxyInterface.
>> 2. I re-implement the KinesisProxy using AWS SDK V2.x.
>> 3. I use the new KinesisProxy to implement ShardConsumer.
>>
>> Though my design is not fully considered, I hope we can discuss a
>> little
>> bit about this feature. I wish to make some contribution to the
community.
>>
>>
>>
>>
>> --
>> Sent from:
>> http://apache-flink-mailing-list-archive.1008284.n3.nabble.com/
>>
>
