kafka-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Matt Farmer <m...@frmr.me>
Subject Re: [DISCUSS] KIP-275 - Indicate "isClosing" in the SinkTaskContext
Date Tue, 03 Apr 2018 17:46:24 GMT
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 <rhauch@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 <matt@frmr.me> wrote:
>
> > I have made the requested updates to the KIP! :)
> >
> > On Mon, Apr 2, 2018 at 11:02 AM, Matt Farmer <matt@frmr.me> wrote:
> >
> > > Ugh
> > >
> > > * I can update
> > >
> > > I need more caffeine...
> > >
> > > On Mon, Apr 2, 2018 at 11:01 AM, Matt Farmer <matt@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 <rhauch@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 <yuzhihong@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 <matt@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 <yuzhihong@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 <matt@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!
> > >>> > > > >
> > >>> > > >
> > >>> > >
> > >>> >
> > >>>
> > >>
> > >>
> > >
> >
>

Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message