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! > > > > > > > > > >
