I have made the requested updates to the KIP! :) On Mon, Apr 2, 2018 at 11:02 AM, Matt Farmer <m...@frmr.me> wrote:
> Ugh > > * I can update > > I need more caffeine... > > On Mon, Apr 2, 2018 at 11:01 AM, Matt Farmer <m...@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 <rha...@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 <yuzhih...@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 <m...@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 <yuzhih...@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 <m...@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! >>> > > > > >>> > > > >>> > > >>> > >>> >> >> >
