kafka-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Randall Hauch <rha...@gmail.com>
Subject Re: [DISCUSS] KIP-275 - Indicate "isClosing" in the SinkTaskContext
Date Mon, 02 Apr 2018 14:48:44 GMT
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