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