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