You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@kafka.apache.org by Randall Hauch <rh...@gmail.com> on 2018/04/02 14:48:44 UTC
Re: [DISCUSS] KIP-275 - Indicate "isClosing" in the SinkTaskContext
Thanks for the KIP proposal, Matt.
You mention in the "Rejected Alternatives" section that you considered
changing the signature of the `preCommit` method but rejected it because it
would break backward compatibility. Strictly speaking, it is possible to do
this without breaking compatibility by introducing a new `preCommit`
method, deprecating the old one, and having the new implementation call the
old one. Such an approach would be complicated, and I'm not sure it adds
any value. In fact, one of the benefits of having a context object is that
we can add methods like the one you're proposing without causing any
compatibility issues. Anyway, it probably is worth updating this rejected
alternative to be a bit more precise.
Otherwise, I think this is a good approach, though I'd request that we
update the `preCommit` JavaDoc to add a paragraph that explains this
scenario. Thoughts?
Randall
On Wed, Mar 28, 2018 at 9:29 PM, Ted Yu <yu...@gmail.com> wrote:
> I looked at WorkerSinkTask and it seems using a boolean for KIP-275 should
> suffice for now.
>
> Thanks
>
> On Wed, Mar 28, 2018 at 7:20 PM, Matt Farmer <ma...@frmr.me> wrote:
>
> > Hey Ted,
> >
> > I have not, actually!
> >
> > Do you think that we're likely to add multiple states here soon?
> >
> > My instinct is to keep it simple until there are multiple states that we
> > would want
> > to consider. I really like the simplicity of just getting a boolean and
> the
> > implementation of WorkerSinkTask already passes around a boolean to
> > indicate this is happening internally. We're really just shuttling that
> > value into
> > the context at the correct moments.
> >
> > Once we have multiple states, we could choose to provide a more
> > appropriately
> > named method (e.g. getState?) and reimplement isClosing by checking that
> > enum
> > without breaking compatibility.
> >
> > However, if we think multiple states here are imminent for some reason, I
> > would
> > be pretty easy to convince adding that would be worth the extra
> complexity!
> > :)
> >
> > Matt
> >
> > —
> > Matt Farmer | Blog <http://farmdawgnation.com/> | Twitter
> > <http://twitter.com/farmdawgnation>
> > GPG: CD57 2E26 F60C 0A61 E6D8 FC72 4493 8917 D667 4D07
> >
> > On Wed, Mar 28, 2018 at 10:02 PM, Ted Yu <yu...@gmail.com> wrote:
> >
> > > The enhancement gives SinkTaskContext state information.
> > >
> > > Have you thought of exposing the state retrieval as an enum (initially
> > with
> > > two values) ?
> > >
> > > Thanks
> > >
> > > On Wed, Mar 28, 2018 at 6:55 PM, Matt Farmer <ma...@frmr.me> wrote:
> > >
> > > > Hello all,
> > > >
> > > > I am proposing KIP-275 to improve Connect's SinkTaskContext so that
> > Sinks
> > > > can be informed
> > > > in their preCommit hook if the hook is being invoked as a part of a
> > > > rebalance or Connect
> > > > shutdown.
> > > >
> > > > The KIP is here:
> > > > https://cwiki.apache.org/confluence/pages/viewpage.
> > > action?pageId=75977607
> > > >
> > > > Please let me know what feedback y'all have. Thanks!
> > > >
> > >
> >
>
Re: [DISCUSS] KIP-275 - Indicate "isClosing" in the SinkTaskContext
Posted by Matt Farmer <ma...@frmr.me>.
Given that the conversation has lingered for a bit, I've gone ahead and
opened up a PR with the initial implementation. Let me know your thoughts!
https://github.com/apache/kafka/pull/5002
Also, voting is open - so if you like this idea please send me some binding
+1's before May 22nd so we can get this in Kafka 2.0 :)
On Tue, Apr 17, 2018 at 7:11 PM, Matt Farmer <ma...@frmr.me> wrote:
> Hello all, I've updated a KIP again to add a few sentences about the
> general problem we were facing in the motivation section. Please let me
> know if there is any further feedback.
>
> On Tue, Apr 3, 2018 at 1:46 PM, Matt Farmer <ma...@frmr.me> wrote:
>
>> Hey Randall,
>>
>> Devil's advocate sparring is always a fun game so I'm down. =)
>>
>> Rebalance caused by connectivity failure is the case that caused us to
>> notice the issue. But the issue
>> is really more about giving connectors the tools to facilitate rebalances
>> or a Kafka connect shutdown
>> cleanly. Perhaps that wasn't clear in the KIP.
>>
>> In our case timeouts were *not* uniformly affecting tasks. But every
>> time a timeout occurred in one task,
>> all tasks lost whatever forward progress they had made. So, yes, in the
>> specific case of timeouts a
>> backoff jitter in the connector is a solution for that particular issue.
>> However, this KIP *also* gives connectors
>> enough information to behave intelligently during any kind of rebalance
>> or shutdown because they're able
>> to discover that preCommit is being invoked for that specific reason. (As
>> opposed to being invoked
>> during normal operation.)
>>
>> On Tue, Apr 3, 2018 at 12:36 PM, Randall Hauch <rh...@gmail.com> wrote:
>>
>>> Matt,
>>>
>>> Let me play devil's advocate. Do we need this additional complexity? The
>>> motivation section talked about needing to deal with task failures due to
>>> connectivity problems. Generally speaking, isn't it possible that if one
>>> task has connectivity problems with either the broker or the external
>>> system that other tasks would as well? And in the particular case of S3,
>>> is
>>> it possible to try and prevent the task shutdown in the first place,
>>> perhaps by improving how the S3 connector retries? (We did this in the
>>> Elasticsearch connector using backoff with jitter; see
>>> https://github.com/confluentinc/kafka-connect-elasticsearch/pull/116 for
>>> details.)
>>>
>>> Best regards,
>>>
>>> Randall
>>>
>>> On Tue, Apr 3, 2018 at 8:39 AM, Matt Farmer <ma...@frmr.me> wrote:
>>>
>>> > I have made the requested updates to the KIP! :)
>>> >
>>> > On Mon, Apr 2, 2018 at 11:02 AM, Matt Farmer <ma...@frmr.me> wrote:
>>> >
>>> > > Ugh
>>> > >
>>> > > * I can update
>>> > >
>>> > > I need more caffeine...
>>> > >
>>> > > On Mon, Apr 2, 2018 at 11:01 AM, Matt Farmer <ma...@frmr.me> wrote:
>>> > >
>>> > >> I'm can update the rejected alternatives section as you describe.
>>> > >>
>>> > >> Also, adding a paragraph to the preCommit javadoc also seems like a
>>> > >> Very Very Good Idea™ so I'll make that update to the KIP as well.
>>> > >>
>>> > >> On Mon, Apr 2, 2018 at 10:48 AM, Randall Hauch <rh...@gmail.com>
>>> > wrote:
>>> > >>
>>> > >>> Thanks for the KIP proposal, Matt.
>>> > >>>
>>> > >>> You mention in the "Rejected Alternatives" section that you
>>> considered
>>> > >>> changing the signature of the `preCommit` method but rejected it
>>> > because
>>> > >>> it
>>> > >>> would break backward compatibility. Strictly speaking, it is
>>> possible
>>> > to
>>> > >>> do
>>> > >>> this without breaking compatibility by introducing a new
>>> `preCommit`
>>> > >>> method, deprecating the old one, and having the new implementation
>>> call
>>> > >>> the
>>> > >>> old one. Such an approach would be complicated, and I'm not sure it
>>> > adds
>>> > >>> any value. In fact, one of the benefits of having a context object
>>> is
>>> > >>> that
>>> > >>> we can add methods like the one you're proposing without causing
>>> any
>>> > >>> compatibility issues. Anyway, it probably is worth updating this
>>> > rejected
>>> > >>> alternative to be a bit more precise.
>>> > >>>
>>> > >>> Otherwise, I think this is a good approach, though I'd request
>>> that we
>>> > >>> update the `preCommit` JavaDoc to add a paragraph that explains
>>> this
>>> > >>> scenario. Thoughts?
>>> > >>>
>>> > >>> Randall
>>> > >>>
>>> > >>> On Wed, Mar 28, 2018 at 9:29 PM, Ted Yu <yu...@gmail.com>
>>> wrote:
>>> > >>>
>>> > >>> > I looked at WorkerSinkTask and it seems using a boolean for
>>> KIP-275
>>> > >>> should
>>> > >>> > suffice for now.
>>> > >>> >
>>> > >>> > Thanks
>>> > >>> >
>>> > >>> > On Wed, Mar 28, 2018 at 7:20 PM, Matt Farmer <ma...@frmr.me>
>>> wrote:
>>> > >>> >
>>> > >>> > > Hey Ted,
>>> > >>> > >
>>> > >>> > > I have not, actually!
>>> > >>> > >
>>> > >>> > > Do you think that we're likely to add multiple states here
>>> soon?
>>> > >>> > >
>>> > >>> > > My instinct is to keep it simple until there are multiple
>>> states
>>> > >>> that we
>>> > >>> > > would want
>>> > >>> > > to consider. I really like the simplicity of just getting a
>>> boolean
>>> > >>> and
>>> > >>> > the
>>> > >>> > > implementation of WorkerSinkTask already passes around a
>>> boolean to
>>> > >>> > > indicate this is happening internally. We're really just
>>> shuttling
>>> > >>> that
>>> > >>> > > value into
>>> > >>> > > the context at the correct moments.
>>> > >>> > >
>>> > >>> > > Once we have multiple states, we could choose to provide a more
>>> > >>> > > appropriately
>>> > >>> > > named method (e.g. getState?) and reimplement isClosing by
>>> checking
>>> > >>> that
>>> > >>> > > enum
>>> > >>> > > without breaking compatibility.
>>> > >>> > >
>>> > >>> > > However, if we think multiple states here are imminent for some
>>> > >>> reason, I
>>> > >>> > > would
>>> > >>> > > be pretty easy to convince adding that would be worth the extra
>>> > >>> > complexity!
>>> > >>> > > :)
>>> > >>> > >
>>> > >>> > > Matt
>>> > >>> > >
>>> > >>> > > —
>>> > >>> > > Matt Farmer | Blog <http://farmdawgnation.com/> | Twitter
>>> > >>> > > <http://twitter.com/farmdawgnation>
>>> > >>> > > GPG: CD57 2E26 F60C 0A61 E6D8 FC72 4493 8917 D667 4D07
>>> > >>> > >
>>> > >>> > > On Wed, Mar 28, 2018 at 10:02 PM, Ted Yu <yu...@gmail.com>
>>> > >>> wrote:
>>> > >>> > >
>>> > >>> > > > The enhancement gives SinkTaskContext state information.
>>> > >>> > > >
>>> > >>> > > > Have you thought of exposing the state retrieval as an enum
>>> > >>> (initially
>>> > >>> > > with
>>> > >>> > > > two values) ?
>>> > >>> > > >
>>> > >>> > > > Thanks
>>> > >>> > > >
>>> > >>> > > > On Wed, Mar 28, 2018 at 6:55 PM, Matt Farmer <ma...@frmr.me>
>>> > wrote:
>>> > >>> > > >
>>> > >>> > > > > Hello all,
>>> > >>> > > > >
>>> > >>> > > > > I am proposing KIP-275 to improve Connect's
>>> SinkTaskContext so
>>> > >>> that
>>> > >>> > > Sinks
>>> > >>> > > > > can be informed
>>> > >>> > > > > in their preCommit hook if the hook is being invoked as a
>>> part
>>> > >>> of a
>>> > >>> > > > > rebalance or Connect
>>> > >>> > > > > shutdown.
>>> > >>> > > > >
>>> > >>> > > > > The KIP is here:
>>> > >>> > > > > https://cwiki.apache.org/confluence/pages/viewpage.
>>> > >>> > > > action?pageId=75977607
>>> > >>> > > > >
>>> > >>> > > > > Please let me know what feedback y'all have. Thanks!
>>> > >>> > > > >
>>> > >>> > > >
>>> > >>> > >
>>> > >>> >
>>> > >>>
>>> > >>
>>> > >>
>>> > >
>>> >
>>>
>>
>>
>
Re: [DISCUSS] KIP-275 - Indicate "isClosing" in the SinkTaskContext
Posted by Matt Farmer <ma...@frmr.me>.
Hello all, I've updated a KIP again to add a few sentences about the
general problem we were facing in the motivation section. Please let me
know if there is any further feedback.
On Tue, Apr 3, 2018 at 1:46 PM, Matt Farmer <ma...@frmr.me> wrote:
> Hey Randall,
>
> Devil's advocate sparring is always a fun game so I'm down. =)
>
> Rebalance caused by connectivity failure is the case that caused us to
> notice the issue. But the issue
> is really more about giving connectors the tools to facilitate rebalances
> or a Kafka connect shutdown
> cleanly. Perhaps that wasn't clear in the KIP.
>
> In our case timeouts were *not* uniformly affecting tasks. But every time
> a timeout occurred in one task,
> all tasks lost whatever forward progress they had made. So, yes, in the
> specific case of timeouts a
> backoff jitter in the connector is a solution for that particular issue.
> However, this KIP *also* gives connectors
> enough information to behave intelligently during any kind of rebalance or
> shutdown because they're able
> to discover that preCommit is being invoked for that specific reason. (As
> opposed to being invoked
> during normal operation.)
>
> On Tue, Apr 3, 2018 at 12:36 PM, Randall Hauch <rh...@gmail.com> wrote:
>
>> Matt,
>>
>> Let me play devil's advocate. Do we need this additional complexity? The
>> motivation section talked about needing to deal with task failures due to
>> connectivity problems. Generally speaking, isn't it possible that if one
>> task has connectivity problems with either the broker or the external
>> system that other tasks would as well? And in the particular case of S3,
>> is
>> it possible to try and prevent the task shutdown in the first place,
>> perhaps by improving how the S3 connector retries? (We did this in the
>> Elasticsearch connector using backoff with jitter; see
>> https://github.com/confluentinc/kafka-connect-elasticsearch/pull/116 for
>> details.)
>>
>> Best regards,
>>
>> Randall
>>
>> On Tue, Apr 3, 2018 at 8:39 AM, Matt Farmer <ma...@frmr.me> wrote:
>>
>> > I have made the requested updates to the KIP! :)
>> >
>> > On Mon, Apr 2, 2018 at 11:02 AM, Matt Farmer <ma...@frmr.me> wrote:
>> >
>> > > Ugh
>> > >
>> > > * I can update
>> > >
>> > > I need more caffeine...
>> > >
>> > > On Mon, Apr 2, 2018 at 11:01 AM, Matt Farmer <ma...@frmr.me> wrote:
>> > >
>> > >> I'm can update the rejected alternatives section as you describe.
>> > >>
>> > >> Also, adding a paragraph to the preCommit javadoc also seems like a
>> > >> Very Very Good Idea™ so I'll make that update to the KIP as well.
>> > >>
>> > >> On Mon, Apr 2, 2018 at 10:48 AM, Randall Hauch <rh...@gmail.com>
>> > wrote:
>> > >>
>> > >>> Thanks for the KIP proposal, Matt.
>> > >>>
>> > >>> You mention in the "Rejected Alternatives" section that you
>> considered
>> > >>> changing the signature of the `preCommit` method but rejected it
>> > because
>> > >>> it
>> > >>> would break backward compatibility. Strictly speaking, it is
>> possible
>> > to
>> > >>> do
>> > >>> this without breaking compatibility by introducing a new `preCommit`
>> > >>> method, deprecating the old one, and having the new implementation
>> call
>> > >>> the
>> > >>> old one. Such an approach would be complicated, and I'm not sure it
>> > adds
>> > >>> any value. In fact, one of the benefits of having a context object
>> is
>> > >>> that
>> > >>> we can add methods like the one you're proposing without causing any
>> > >>> compatibility issues. Anyway, it probably is worth updating this
>> > rejected
>> > >>> alternative to be a bit more precise.
>> > >>>
>> > >>> Otherwise, I think this is a good approach, though I'd request that
>> we
>> > >>> update the `preCommit` JavaDoc to add a paragraph that explains this
>> > >>> scenario. Thoughts?
>> > >>>
>> > >>> Randall
>> > >>>
>> > >>> On Wed, Mar 28, 2018 at 9:29 PM, Ted Yu <yu...@gmail.com>
>> wrote:
>> > >>>
>> > >>> > I looked at WorkerSinkTask and it seems using a boolean for
>> KIP-275
>> > >>> should
>> > >>> > suffice for now.
>> > >>> >
>> > >>> > Thanks
>> > >>> >
>> > >>> > On Wed, Mar 28, 2018 at 7:20 PM, Matt Farmer <ma...@frmr.me>
>> wrote:
>> > >>> >
>> > >>> > > Hey Ted,
>> > >>> > >
>> > >>> > > I have not, actually!
>> > >>> > >
>> > >>> > > Do you think that we're likely to add multiple states here soon?
>> > >>> > >
>> > >>> > > My instinct is to keep it simple until there are multiple states
>> > >>> that we
>> > >>> > > would want
>> > >>> > > to consider. I really like the simplicity of just getting a
>> boolean
>> > >>> and
>> > >>> > the
>> > >>> > > implementation of WorkerSinkTask already passes around a
>> boolean to
>> > >>> > > indicate this is happening internally. We're really just
>> shuttling
>> > >>> that
>> > >>> > > value into
>> > >>> > > the context at the correct moments.
>> > >>> > >
>> > >>> > > Once we have multiple states, we could choose to provide a more
>> > >>> > > appropriately
>> > >>> > > named method (e.g. getState?) and reimplement isClosing by
>> checking
>> > >>> that
>> > >>> > > enum
>> > >>> > > without breaking compatibility.
>> > >>> > >
>> > >>> > > However, if we think multiple states here are imminent for some
>> > >>> reason, I
>> > >>> > > would
>> > >>> > > be pretty easy to convince adding that would be worth the extra
>> > >>> > complexity!
>> > >>> > > :)
>> > >>> > >
>> > >>> > > Matt
>> > >>> > >
>> > >>> > > —
>> > >>> > > Matt Farmer | Blog <http://farmdawgnation.com/> | Twitter
>> > >>> > > <http://twitter.com/farmdawgnation>
>> > >>> > > GPG: CD57 2E26 F60C 0A61 E6D8 FC72 4493 8917 D667 4D07
>> > >>> > >
>> > >>> > > On Wed, Mar 28, 2018 at 10:02 PM, Ted Yu <yu...@gmail.com>
>> > >>> wrote:
>> > >>> > >
>> > >>> > > > The enhancement gives SinkTaskContext state information.
>> > >>> > > >
>> > >>> > > > Have you thought of exposing the state retrieval as an enum
>> > >>> (initially
>> > >>> > > with
>> > >>> > > > two values) ?
>> > >>> > > >
>> > >>> > > > Thanks
>> > >>> > > >
>> > >>> > > > On Wed, Mar 28, 2018 at 6:55 PM, Matt Farmer <ma...@frmr.me>
>> > wrote:
>> > >>> > > >
>> > >>> > > > > Hello all,
>> > >>> > > > >
>> > >>> > > > > I am proposing KIP-275 to improve Connect's SinkTaskContext
>> so
>> > >>> that
>> > >>> > > Sinks
>> > >>> > > > > can be informed
>> > >>> > > > > in their preCommit hook if the hook is being invoked as a
>> part
>> > >>> of a
>> > >>> > > > > rebalance or Connect
>> > >>> > > > > shutdown.
>> > >>> > > > >
>> > >>> > > > > The KIP is here:
>> > >>> > > > > https://cwiki.apache.org/confluence/pages/viewpage.
>> > >>> > > > action?pageId=75977607
>> > >>> > > > >
>> > >>> > > > > Please let me know what feedback y'all have. Thanks!
>> > >>> > > > >
>> > >>> > > >
>> > >>> > >
>> > >>> >
>> > >>>
>> > >>
>> > >>
>> > >
>> >
>>
>
>
Re: [DISCUSS] KIP-275 - Indicate "isClosing" in the SinkTaskContext
Posted by Matt Farmer <ma...@frmr.me>.
Hey Randall,
Devil's advocate sparring is always a fun game so I'm down. =)
Rebalance caused by connectivity failure is the case that caused us to
notice the issue. But the issue
is really more about giving connectors the tools to facilitate rebalances
or a Kafka connect shutdown
cleanly. Perhaps that wasn't clear in the KIP.
In our case timeouts were *not* uniformly affecting tasks. But every time a
timeout occurred in one task,
all tasks lost whatever forward progress they had made. So, yes, in the
specific case of timeouts a
backoff jitter in the connector is a solution for that particular issue.
However, this KIP *also* gives connectors
enough information to behave intelligently during any kind of rebalance or
shutdown because they're able
to discover that preCommit is being invoked for that specific reason. (As
opposed to being invoked
during normal operation.)
On Tue, Apr 3, 2018 at 12:36 PM, Randall Hauch <rh...@gmail.com> wrote:
> Matt,
>
> Let me play devil's advocate. Do we need this additional complexity? The
> motivation section talked about needing to deal with task failures due to
> connectivity problems. Generally speaking, isn't it possible that if one
> task has connectivity problems with either the broker or the external
> system that other tasks would as well? And in the particular case of S3, is
> it possible to try and prevent the task shutdown in the first place,
> perhaps by improving how the S3 connector retries? (We did this in the
> Elasticsearch connector using backoff with jitter; see
> https://github.com/confluentinc/kafka-connect-elasticsearch/pull/116 for
> details.)
>
> Best regards,
>
> Randall
>
> On Tue, Apr 3, 2018 at 8:39 AM, Matt Farmer <ma...@frmr.me> wrote:
>
> > I have made the requested updates to the KIP! :)
> >
> > On Mon, Apr 2, 2018 at 11:02 AM, Matt Farmer <ma...@frmr.me> wrote:
> >
> > > Ugh
> > >
> > > * I can update
> > >
> > > I need more caffeine...
> > >
> > > On Mon, Apr 2, 2018 at 11:01 AM, Matt Farmer <ma...@frmr.me> wrote:
> > >
> > >> I'm can update the rejected alternatives section as you describe.
> > >>
> > >> Also, adding a paragraph to the preCommit javadoc also seems like a
> > >> Very Very Good Idea™ so I'll make that update to the KIP as well.
> > >>
> > >> On Mon, Apr 2, 2018 at 10:48 AM, Randall Hauch <rh...@gmail.com>
> > wrote:
> > >>
> > >>> Thanks for the KIP proposal, Matt.
> > >>>
> > >>> You mention in the "Rejected Alternatives" section that you
> considered
> > >>> changing the signature of the `preCommit` method but rejected it
> > because
> > >>> it
> > >>> would break backward compatibility. Strictly speaking, it is possible
> > to
> > >>> do
> > >>> this without breaking compatibility by introducing a new `preCommit`
> > >>> method, deprecating the old one, and having the new implementation
> call
> > >>> the
> > >>> old one. Such an approach would be complicated, and I'm not sure it
> > adds
> > >>> any value. In fact, one of the benefits of having a context object is
> > >>> that
> > >>> we can add methods like the one you're proposing without causing any
> > >>> compatibility issues. Anyway, it probably is worth updating this
> > rejected
> > >>> alternative to be a bit more precise.
> > >>>
> > >>> Otherwise, I think this is a good approach, though I'd request that
> we
> > >>> update the `preCommit` JavaDoc to add a paragraph that explains this
> > >>> scenario. Thoughts?
> > >>>
> > >>> Randall
> > >>>
> > >>> On Wed, Mar 28, 2018 at 9:29 PM, Ted Yu <yu...@gmail.com> wrote:
> > >>>
> > >>> > I looked at WorkerSinkTask and it seems using a boolean for KIP-275
> > >>> should
> > >>> > suffice for now.
> > >>> >
> > >>> > Thanks
> > >>> >
> > >>> > On Wed, Mar 28, 2018 at 7:20 PM, Matt Farmer <ma...@frmr.me> wrote:
> > >>> >
> > >>> > > Hey Ted,
> > >>> > >
> > >>> > > I have not, actually!
> > >>> > >
> > >>> > > Do you think that we're likely to add multiple states here soon?
> > >>> > >
> > >>> > > My instinct is to keep it simple until there are multiple states
> > >>> that we
> > >>> > > would want
> > >>> > > to consider. I really like the simplicity of just getting a
> boolean
> > >>> and
> > >>> > the
> > >>> > > implementation of WorkerSinkTask already passes around a boolean
> to
> > >>> > > indicate this is happening internally. We're really just
> shuttling
> > >>> that
> > >>> > > value into
> > >>> > > the context at the correct moments.
> > >>> > >
> > >>> > > Once we have multiple states, we could choose to provide a more
> > >>> > > appropriately
> > >>> > > named method (e.g. getState?) and reimplement isClosing by
> checking
> > >>> that
> > >>> > > enum
> > >>> > > without breaking compatibility.
> > >>> > >
> > >>> > > However, if we think multiple states here are imminent for some
> > >>> reason, I
> > >>> > > would
> > >>> > > be pretty easy to convince adding that would be worth the extra
> > >>> > complexity!
> > >>> > > :)
> > >>> > >
> > >>> > > Matt
> > >>> > >
> > >>> > > —
> > >>> > > Matt Farmer | Blog <http://farmdawgnation.com/> | Twitter
> > >>> > > <http://twitter.com/farmdawgnation>
> > >>> > > GPG: CD57 2E26 F60C 0A61 E6D8 FC72 4493 8917 D667 4D07
> > >>> > >
> > >>> > > On Wed, Mar 28, 2018 at 10:02 PM, Ted Yu <yu...@gmail.com>
> > >>> wrote:
> > >>> > >
> > >>> > > > The enhancement gives SinkTaskContext state information.
> > >>> > > >
> > >>> > > > Have you thought of exposing the state retrieval as an enum
> > >>> (initially
> > >>> > > with
> > >>> > > > two values) ?
> > >>> > > >
> > >>> > > > Thanks
> > >>> > > >
> > >>> > > > On Wed, Mar 28, 2018 at 6:55 PM, Matt Farmer <ma...@frmr.me>
> > wrote:
> > >>> > > >
> > >>> > > > > Hello all,
> > >>> > > > >
> > >>> > > > > I am proposing KIP-275 to improve Connect's SinkTaskContext
> so
> > >>> that
> > >>> > > Sinks
> > >>> > > > > can be informed
> > >>> > > > > in their preCommit hook if the hook is being invoked as a
> part
> > >>> of a
> > >>> > > > > rebalance or Connect
> > >>> > > > > shutdown.
> > >>> > > > >
> > >>> > > > > The KIP is here:
> > >>> > > > > https://cwiki.apache.org/confluence/pages/viewpage.
> > >>> > > > action?pageId=75977607
> > >>> > > > >
> > >>> > > > > Please let me know what feedback y'all have. Thanks!
> > >>> > > > >
> > >>> > > >
> > >>> > >
> > >>> >
> > >>>
> > >>
> > >>
> > >
> >
>
Re: [DISCUSS] KIP-275 - Indicate "isClosing" in the SinkTaskContext
Posted by Randall Hauch <rh...@gmail.com>.
Matt,
Let me play devil's advocate. Do we need this additional complexity? The
motivation section talked about needing to deal with task failures due to
connectivity problems. Generally speaking, isn't it possible that if one
task has connectivity problems with either the broker or the external
system that other tasks would as well? And in the particular case of S3, is
it possible to try and prevent the task shutdown in the first place,
perhaps by improving how the S3 connector retries? (We did this in the
Elasticsearch connector using backoff with jitter; see
https://github.com/confluentinc/kafka-connect-elasticsearch/pull/116 for
details.)
Best regards,
Randall
On Tue, Apr 3, 2018 at 8:39 AM, Matt Farmer <ma...@frmr.me> wrote:
> I have made the requested updates to the KIP! :)
>
> On Mon, Apr 2, 2018 at 11:02 AM, Matt Farmer <ma...@frmr.me> wrote:
>
> > Ugh
> >
> > * I can update
> >
> > I need more caffeine...
> >
> > On Mon, Apr 2, 2018 at 11:01 AM, Matt Farmer <ma...@frmr.me> wrote:
> >
> >> I'm can update the rejected alternatives section as you describe.
> >>
> >> Also, adding a paragraph to the preCommit javadoc also seems like a
> >> Very Very Good Idea™ so I'll make that update to the KIP as well.
> >>
> >> On Mon, Apr 2, 2018 at 10:48 AM, Randall Hauch <rh...@gmail.com>
> wrote:
> >>
> >>> Thanks for the KIP proposal, Matt.
> >>>
> >>> You mention in the "Rejected Alternatives" section that you considered
> >>> changing the signature of the `preCommit` method but rejected it
> because
> >>> it
> >>> would break backward compatibility. Strictly speaking, it is possible
> to
> >>> do
> >>> this without breaking compatibility by introducing a new `preCommit`
> >>> method, deprecating the old one, and having the new implementation call
> >>> the
> >>> old one. Such an approach would be complicated, and I'm not sure it
> adds
> >>> any value. In fact, one of the benefits of having a context object is
> >>> that
> >>> we can add methods like the one you're proposing without causing any
> >>> compatibility issues. Anyway, it probably is worth updating this
> rejected
> >>> alternative to be a bit more precise.
> >>>
> >>> Otherwise, I think this is a good approach, though I'd request that we
> >>> update the `preCommit` JavaDoc to add a paragraph that explains this
> >>> scenario. Thoughts?
> >>>
> >>> Randall
> >>>
> >>> On Wed, Mar 28, 2018 at 9:29 PM, Ted Yu <yu...@gmail.com> wrote:
> >>>
> >>> > I looked at WorkerSinkTask and it seems using a boolean for KIP-275
> >>> should
> >>> > suffice for now.
> >>> >
> >>> > Thanks
> >>> >
> >>> > On Wed, Mar 28, 2018 at 7:20 PM, Matt Farmer <ma...@frmr.me> wrote:
> >>> >
> >>> > > Hey Ted,
> >>> > >
> >>> > > I have not, actually!
> >>> > >
> >>> > > Do you think that we're likely to add multiple states here soon?
> >>> > >
> >>> > > My instinct is to keep it simple until there are multiple states
> >>> that we
> >>> > > would want
> >>> > > to consider. I really like the simplicity of just getting a boolean
> >>> and
> >>> > the
> >>> > > implementation of WorkerSinkTask already passes around a boolean to
> >>> > > indicate this is happening internally. We're really just shuttling
> >>> that
> >>> > > value into
> >>> > > the context at the correct moments.
> >>> > >
> >>> > > Once we have multiple states, we could choose to provide a more
> >>> > > appropriately
> >>> > > named method (e.g. getState?) and reimplement isClosing by checking
> >>> that
> >>> > > enum
> >>> > > without breaking compatibility.
> >>> > >
> >>> > > However, if we think multiple states here are imminent for some
> >>> reason, I
> >>> > > would
> >>> > > be pretty easy to convince adding that would be worth the extra
> >>> > complexity!
> >>> > > :)
> >>> > >
> >>> > > Matt
> >>> > >
> >>> > > —
> >>> > > Matt Farmer | Blog <http://farmdawgnation.com/> | Twitter
> >>> > > <http://twitter.com/farmdawgnation>
> >>> > > GPG: CD57 2E26 F60C 0A61 E6D8 FC72 4493 8917 D667 4D07
> >>> > >
> >>> > > On Wed, Mar 28, 2018 at 10:02 PM, Ted Yu <yu...@gmail.com>
> >>> wrote:
> >>> > >
> >>> > > > The enhancement gives SinkTaskContext state information.
> >>> > > >
> >>> > > > Have you thought of exposing the state retrieval as an enum
> >>> (initially
> >>> > > with
> >>> > > > two values) ?
> >>> > > >
> >>> > > > Thanks
> >>> > > >
> >>> > > > On Wed, Mar 28, 2018 at 6:55 PM, Matt Farmer <ma...@frmr.me>
> wrote:
> >>> > > >
> >>> > > > > Hello all,
> >>> > > > >
> >>> > > > > I am proposing KIP-275 to improve Connect's SinkTaskContext so
> >>> that
> >>> > > Sinks
> >>> > > > > can be informed
> >>> > > > > in their preCommit hook if the hook is being invoked as a part
> >>> of a
> >>> > > > > rebalance or Connect
> >>> > > > > shutdown.
> >>> > > > >
> >>> > > > > The KIP is here:
> >>> > > > > https://cwiki.apache.org/confluence/pages/viewpage.
> >>> > > > action?pageId=75977607
> >>> > > > >
> >>> > > > > Please let me know what feedback y'all have. Thanks!
> >>> > > > >
> >>> > > >
> >>> > >
> >>> >
> >>>
> >>
> >>
> >
>
Re: [DISCUSS] KIP-275 - Indicate "isClosing" in the SinkTaskContext
Posted by Matt Farmer <ma...@frmr.me>.
I have made the requested updates to the KIP! :)
On Mon, Apr 2, 2018 at 11:02 AM, Matt Farmer <ma...@frmr.me> wrote:
> Ugh
>
> * I can update
>
> I need more caffeine...
>
> On Mon, Apr 2, 2018 at 11:01 AM, Matt Farmer <ma...@frmr.me> wrote:
>
>> I'm can update the rejected alternatives section as you describe.
>>
>> Also, adding a paragraph to the preCommit javadoc also seems like a
>> Very Very Good Idea™ so I'll make that update to the KIP as well.
>>
>> On Mon, Apr 2, 2018 at 10:48 AM, Randall Hauch <rh...@gmail.com> wrote:
>>
>>> Thanks for the KIP proposal, Matt.
>>>
>>> You mention in the "Rejected Alternatives" section that you considered
>>> changing the signature of the `preCommit` method but rejected it because
>>> it
>>> would break backward compatibility. Strictly speaking, it is possible to
>>> do
>>> this without breaking compatibility by introducing a new `preCommit`
>>> method, deprecating the old one, and having the new implementation call
>>> the
>>> old one. Such an approach would be complicated, and I'm not sure it adds
>>> any value. In fact, one of the benefits of having a context object is
>>> that
>>> we can add methods like the one you're proposing without causing any
>>> compatibility issues. Anyway, it probably is worth updating this rejected
>>> alternative to be a bit more precise.
>>>
>>> Otherwise, I think this is a good approach, though I'd request that we
>>> update the `preCommit` JavaDoc to add a paragraph that explains this
>>> scenario. Thoughts?
>>>
>>> Randall
>>>
>>> On Wed, Mar 28, 2018 at 9:29 PM, Ted Yu <yu...@gmail.com> wrote:
>>>
>>> > I looked at WorkerSinkTask and it seems using a boolean for KIP-275
>>> should
>>> > suffice for now.
>>> >
>>> > Thanks
>>> >
>>> > On Wed, Mar 28, 2018 at 7:20 PM, Matt Farmer <ma...@frmr.me> wrote:
>>> >
>>> > > Hey Ted,
>>> > >
>>> > > I have not, actually!
>>> > >
>>> > > Do you think that we're likely to add multiple states here soon?
>>> > >
>>> > > My instinct is to keep it simple until there are multiple states
>>> that we
>>> > > would want
>>> > > to consider. I really like the simplicity of just getting a boolean
>>> and
>>> > the
>>> > > implementation of WorkerSinkTask already passes around a boolean to
>>> > > indicate this is happening internally. We're really just shuttling
>>> that
>>> > > value into
>>> > > the context at the correct moments.
>>> > >
>>> > > Once we have multiple states, we could choose to provide a more
>>> > > appropriately
>>> > > named method (e.g. getState?) and reimplement isClosing by checking
>>> that
>>> > > enum
>>> > > without breaking compatibility.
>>> > >
>>> > > However, if we think multiple states here are imminent for some
>>> reason, I
>>> > > would
>>> > > be pretty easy to convince adding that would be worth the extra
>>> > complexity!
>>> > > :)
>>> > >
>>> > > Matt
>>> > >
>>> > > —
>>> > > Matt Farmer | Blog <http://farmdawgnation.com/> | Twitter
>>> > > <http://twitter.com/farmdawgnation>
>>> > > GPG: CD57 2E26 F60C 0A61 E6D8 FC72 4493 8917 D667 4D07
>>> > >
>>> > > On Wed, Mar 28, 2018 at 10:02 PM, Ted Yu <yu...@gmail.com>
>>> wrote:
>>> > >
>>> > > > The enhancement gives SinkTaskContext state information.
>>> > > >
>>> > > > Have you thought of exposing the state retrieval as an enum
>>> (initially
>>> > > with
>>> > > > two values) ?
>>> > > >
>>> > > > Thanks
>>> > > >
>>> > > > On Wed, Mar 28, 2018 at 6:55 PM, Matt Farmer <ma...@frmr.me> wrote:
>>> > > >
>>> > > > > Hello all,
>>> > > > >
>>> > > > > I am proposing KIP-275 to improve Connect's SinkTaskContext so
>>> that
>>> > > Sinks
>>> > > > > can be informed
>>> > > > > in their preCommit hook if the hook is being invoked as a part
>>> of a
>>> > > > > rebalance or Connect
>>> > > > > shutdown.
>>> > > > >
>>> > > > > The KIP is here:
>>> > > > > https://cwiki.apache.org/confluence/pages/viewpage.
>>> > > > action?pageId=75977607
>>> > > > >
>>> > > > > Please let me know what feedback y'all have. Thanks!
>>> > > > >
>>> > > >
>>> > >
>>> >
>>>
>>
>>
>
Re: [DISCUSS] KIP-275 - Indicate "isClosing" in the SinkTaskContext
Posted by Matt Farmer <ma...@frmr.me>.
Ugh
* I can update
I need more caffeine...
On Mon, Apr 2, 2018 at 11:01 AM, Matt Farmer <ma...@frmr.me> wrote:
> I'm can update the rejected alternatives section as you describe.
>
> Also, adding a paragraph to the preCommit javadoc also seems like a
> Very Very Good Idea™ so I'll make that update to the KIP as well.
>
> On Mon, Apr 2, 2018 at 10:48 AM, Randall Hauch <rh...@gmail.com> wrote:
>
>> Thanks for the KIP proposal, Matt.
>>
>> You mention in the "Rejected Alternatives" section that you considered
>> changing the signature of the `preCommit` method but rejected it because
>> it
>> would break backward compatibility. Strictly speaking, it is possible to
>> do
>> this without breaking compatibility by introducing a new `preCommit`
>> method, deprecating the old one, and having the new implementation call
>> the
>> old one. Such an approach would be complicated, and I'm not sure it adds
>> any value. In fact, one of the benefits of having a context object is that
>> we can add methods like the one you're proposing without causing any
>> compatibility issues. Anyway, it probably is worth updating this rejected
>> alternative to be a bit more precise.
>>
>> Otherwise, I think this is a good approach, though I'd request that we
>> update the `preCommit` JavaDoc to add a paragraph that explains this
>> scenario. Thoughts?
>>
>> Randall
>>
>> On Wed, Mar 28, 2018 at 9:29 PM, Ted Yu <yu...@gmail.com> wrote:
>>
>> > I looked at WorkerSinkTask and it seems using a boolean for KIP-275
>> should
>> > suffice for now.
>> >
>> > Thanks
>> >
>> > On Wed, Mar 28, 2018 at 7:20 PM, Matt Farmer <ma...@frmr.me> wrote:
>> >
>> > > Hey Ted,
>> > >
>> > > I have not, actually!
>> > >
>> > > Do you think that we're likely to add multiple states here soon?
>> > >
>> > > My instinct is to keep it simple until there are multiple states that
>> we
>> > > would want
>> > > to consider. I really like the simplicity of just getting a boolean
>> and
>> > the
>> > > implementation of WorkerSinkTask already passes around a boolean to
>> > > indicate this is happening internally. We're really just shuttling
>> that
>> > > value into
>> > > the context at the correct moments.
>> > >
>> > > Once we have multiple states, we could choose to provide a more
>> > > appropriately
>> > > named method (e.g. getState?) and reimplement isClosing by checking
>> that
>> > > enum
>> > > without breaking compatibility.
>> > >
>> > > However, if we think multiple states here are imminent for some
>> reason, I
>> > > would
>> > > be pretty easy to convince adding that would be worth the extra
>> > complexity!
>> > > :)
>> > >
>> > > Matt
>> > >
>> > > —
>> > > Matt Farmer | Blog <http://farmdawgnation.com/> | Twitter
>> > > <http://twitter.com/farmdawgnation>
>> > > GPG: CD57 2E26 F60C 0A61 E6D8 FC72 4493 8917 D667 4D07
>> > >
>> > > On Wed, Mar 28, 2018 at 10:02 PM, Ted Yu <yu...@gmail.com> wrote:
>> > >
>> > > > The enhancement gives SinkTaskContext state information.
>> > > >
>> > > > Have you thought of exposing the state retrieval as an enum
>> (initially
>> > > with
>> > > > two values) ?
>> > > >
>> > > > Thanks
>> > > >
>> > > > On Wed, Mar 28, 2018 at 6:55 PM, Matt Farmer <ma...@frmr.me> wrote:
>> > > >
>> > > > > Hello all,
>> > > > >
>> > > > > I am proposing KIP-275 to improve Connect's SinkTaskContext so
>> that
>> > > Sinks
>> > > > > can be informed
>> > > > > in their preCommit hook if the hook is being invoked as a part of
>> a
>> > > > > rebalance or Connect
>> > > > > shutdown.
>> > > > >
>> > > > > The KIP is here:
>> > > > > https://cwiki.apache.org/confluence/pages/viewpage.
>> > > > action?pageId=75977607
>> > > > >
>> > > > > Please let me know what feedback y'all have. Thanks!
>> > > > >
>> > > >
>> > >
>> >
>>
>
>
Re: [DISCUSS] KIP-275 - Indicate "isClosing" in the SinkTaskContext
Posted by Matt Farmer <ma...@frmr.me>.
I'm can update the rejected alternatives section as you describe.
Also, adding a paragraph to the preCommit javadoc also seems like a
Very Very Good Idea™ so I'll make that update to the KIP as well.
On Mon, Apr 2, 2018 at 10:48 AM, Randall Hauch <rh...@gmail.com> wrote:
> Thanks for the KIP proposal, Matt.
>
> You mention in the "Rejected Alternatives" section that you considered
> changing the signature of the `preCommit` method but rejected it because it
> would break backward compatibility. Strictly speaking, it is possible to do
> this without breaking compatibility by introducing a new `preCommit`
> method, deprecating the old one, and having the new implementation call the
> old one. Such an approach would be complicated, and I'm not sure it adds
> any value. In fact, one of the benefits of having a context object is that
> we can add methods like the one you're proposing without causing any
> compatibility issues. Anyway, it probably is worth updating this rejected
> alternative to be a bit more precise.
>
> Otherwise, I think this is a good approach, though I'd request that we
> update the `preCommit` JavaDoc to add a paragraph that explains this
> scenario. Thoughts?
>
> Randall
>
> On Wed, Mar 28, 2018 at 9:29 PM, Ted Yu <yu...@gmail.com> wrote:
>
> > I looked at WorkerSinkTask and it seems using a boolean for KIP-275
> should
> > suffice for now.
> >
> > Thanks
> >
> > On Wed, Mar 28, 2018 at 7:20 PM, Matt Farmer <ma...@frmr.me> wrote:
> >
> > > Hey Ted,
> > >
> > > I have not, actually!
> > >
> > > Do you think that we're likely to add multiple states here soon?
> > >
> > > My instinct is to keep it simple until there are multiple states that
> we
> > > would want
> > > to consider. I really like the simplicity of just getting a boolean and
> > the
> > > implementation of WorkerSinkTask already passes around a boolean to
> > > indicate this is happening internally. We're really just shuttling that
> > > value into
> > > the context at the correct moments.
> > >
> > > Once we have multiple states, we could choose to provide a more
> > > appropriately
> > > named method (e.g. getState?) and reimplement isClosing by checking
> that
> > > enum
> > > without breaking compatibility.
> > >
> > > However, if we think multiple states here are imminent for some
> reason, I
> > > would
> > > be pretty easy to convince adding that would be worth the extra
> > complexity!
> > > :)
> > >
> > > Matt
> > >
> > > —
> > > Matt Farmer | Blog <http://farmdawgnation.com/> | Twitter
> > > <http://twitter.com/farmdawgnation>
> > > GPG: CD57 2E26 F60C 0A61 E6D8 FC72 4493 8917 D667 4D07
> > >
> > > On Wed, Mar 28, 2018 at 10:02 PM, Ted Yu <yu...@gmail.com> wrote:
> > >
> > > > The enhancement gives SinkTaskContext state information.
> > > >
> > > > Have you thought of exposing the state retrieval as an enum
> (initially
> > > with
> > > > two values) ?
> > > >
> > > > Thanks
> > > >
> > > > On Wed, Mar 28, 2018 at 6:55 PM, Matt Farmer <ma...@frmr.me> wrote:
> > > >
> > > > > Hello all,
> > > > >
> > > > > I am proposing KIP-275 to improve Connect's SinkTaskContext so that
> > > Sinks
> > > > > can be informed
> > > > > in their preCommit hook if the hook is being invoked as a part of a
> > > > > rebalance or Connect
> > > > > shutdown.
> > > > >
> > > > > The KIP is here:
> > > > > https://cwiki.apache.org/confluence/pages/viewpage.
> > > > action?pageId=75977607
> > > > >
> > > > > Please let me know what feedback y'all have. Thanks!
> > > > >
> > > >
> > >
> >
>