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 13:39:29 GMT
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