Thanks, Sophie, That makes sense. Should we add a whole new interface and a separate kind of listener just because global threads don't support restarts _yet_, though?
It seems like that will just widen the API surface area, and in a few more months, there will be no more difference between the two handlers. But people will forever afterward have to register two different handlers to do the same thing. Two alternatives we could consider: 1. Just don't add the "restart" option until it's possible for all threads. This KIP is already accepted with a "restart" option that we know won't be added until KIP-663 is done. Maybe we just wait for KIP-406 as well. But the _rest_ of this KIP can be implemented in the mean time. 2. Just log an error and kill the thread anyway if the handler for the global thread opts to "retry". In general, it seems like the problem at hand is better solved by allowing/disallowing that one option, versus adding a whole new interface. Thanks, -John On Wed, 2020-10-14 at 11:48 -0700, Sophie Blee-Goldman wrote: > I don't think the proposal was to *never* add the "replace" functionality > for > the global thread, but we didn't want to tie up this KIP with anything more. > As I understand it, the goal of Walker's proposal was to set us up for > success if/when we want to add new functionality for the global thread, > without necessarily committing to it at this time. > > Restarting the global thread will take a bit more work since you need to > pause any further work that relies on global state until it's back up. > That's > starting to sound more in the purview of KIP-406 whose current goal > is to effectively restart the global thread on a specific type of exception > (OffsetOutOfRange). If we want to consider expanding that to allow users > to choose to restart the thread, then KIP-406 seems like the more > appropriate place to engage in that discussion. > > On Wed, Oct 14, 2020 at 7:13 AM John Roesler <vvcep...@apache.org> wrote: > > > Hello Walker, > > > > Sorry for the late reply, but I didn’t follow the reasoning for the > > separate handler. You said that the global thread doesn’t have “replace”, > > but as of today, none of the threads have “replace”. Why not add that > > ability when we add it for the other threads? > > > > The nature of an uncaught exception handler is that there is an exception > > that will kill the thread. In that case, it seems like replacement is a > > desirable option. > > > > What have I missed? > > > > Thanks, > > John > > > > On Tue, Oct 13, 2020, at 15:49, Walker Carlson wrote: > > > Those are good points Sophie and Matthias. I sepificed the defaults in > > the > > > kip and standardized the names fo the handler to make them a bit more > > > readable. > > > > > > Thanks for the suggestions, > > > Walker > > > > > > On Tue, Oct 13, 2020 at 12:24 PM Sophie Blee-Goldman < > > sop...@confluent.io> > > > wrote: > > > > > > > Super nit: can we standardize the method & enum names? > > > > > > > > Right now we have these enums: > > > > StreamsUncaughtExceptionHandlerResponse > > > > StreamsUncaughtExceptionHandlerResponseGlobalThread > > > > > > > > and these callbacks: > > > > handleUncaughtException() > > > > handleExceptionInGlobalThread() > > > > > > > > The method names have different syntax, which is a bit clunky. I don't > > have > > > > any > > > > strong opinions on what grammar they should follow, just that it > > should be > > > > the > > > > same for each. I also think that we should specify "StreamThread" > > somewhere > > > > in the name of the StreadThread-specific callback, now that we have a > > > > second > > > > callback that specifies it's for the GlobalThread. Something like > > > > "*handleStreamThreadException()*" and "*handleGlobalThreadException*" > > > > > > > > The enums are ok, although I think we should include "StreamThread" > > > > somewhere > > > > like with the callbacks. And we can probably shorten them a bit. For > > > > example > > > > "*StreamThreadExceptionResponse*" and "*GlobalThreadExceptionResponse*" > > > > > > > > > > > > > > > > On Tue, Oct 13, 2020 at 11:48 AM Matthias J. Sax <mj...@apache.org> > > wrote: > > > > > Thanks Walker. > > > > > > > > > > Overall, LGTM. However, I am wondering if we should have default > > > > > implementations for both handler methods? Before the latest change, > > > > > there was only one method and having a default was not necessary. > > > > > However, forcing people to implement both methods might not be the > > best > > > > > user experience: for example, if there is no global thread, one > > should > > > > > not need to implement the global handler method (and the other way > > > > around). > > > > > Thus, it might be good to add default for both methods. If we add > > > > > defaults, we should explain the default behavior to the KIP. > > > > > > > > > > -Matthias > > > > > > > > > > On 10/12/20 2:32 PM, Walker Carlson wrote: > > > > > > Hello all, > > > > > > > > > > > > I just wanted to let you know that I had to make 2 minor updates > > to the > > > > > KIP. > > > > > > 1) I changed the behavior of the shutdown client to not leave the > > > > client > > > > > in > > > > > > Error but instead close directly because this aligns better with > > our > > > > > > state machine. > > > > > > > > > > > > 2) I added a separate call back for the global thread as it does > > not > > > > have > > > > > > all the options as a streamThread does. i.e. replace. The default > > will > > > > be > > > > > > to close the client. that will also be the only option as that is > > the > > > > > > current behavior for the global thread. > > > > > > > > > > > > you can find the diff here: > > > > > > > > https://cwiki.apache.org/confluence/pages/diffpagesbyversion.action?pageId=158876566&originalVersion=21&revisedVersion=23 > > > > > > If you have any problems with these changes let me know and we can > > > > > discuss > > > > > > them further > > > > > > > > > > > > Thank you, > > > > > > Walker > > > > > > > > > > > > On Wed, Sep 30, 2020 at 7:33 AM Walker Carlson < > > wcarl...@confluent.io> > > > > > > wrote: > > > > > > > > > > > > > Bruno Cadonna <br...@confluent.io> > > > > > > > 4:51 AM (2 hours ago) > > > > > > > to dev > > > > > > > Thank you all for voting! > > > > > > > > > > > > > > This KIP is accepted with +3 binding (Guozhang, Bill, Matthias) > > and +2 > > > > > > > non-binding (Bruno, Leah). > > > > > > > > > > > > > > Matthias, we will take care of the global threads, and for the > > > > > > > replacement that will depend on Kip-663. > > > > > > > > > > > > > > Best, > > > > > > > > > > > > > > On Wed, Sep 30, 2020 at 4:59 AM Bruno Cadonna <br...@confluent.io > > > > > wrote: > > > > > > > > Thanks a lot Walker! > > > > > > > > > > > > > > > > +1 (non-binding) > > > > > > > > > > > > > > > > Best, > > > > > > > > Bruno > > > > > > > > > > > > > > > > On 30.09.20 03:10, Matthias J. Sax wrote: > > > > > > > > > Thanks Walker. The proposed API changes LGTM. > > > > > > > > > > > > > > > > > > +1 (binding) > > > > > > > > > > > > > > > > > > One minor nit: you should also mention the global-thread that > > also > > > > > needs > > > > > > > > > to be shutdown if requested by the user. > > > > > > > > > > > > > > > > > > Minor side question: should we actually terminate a thread and > > > > create > > > > > a > > > > > > > > > new one, or instead revive the existing thread (reusing its > > existing > > > > > > > > ID)? > > > > > > > > > > > > > > > > > > -Matthias > > > > > > > > > > > > > > > > > > On 9/29/20 2:39 PM, Bill Bejeck wrote: > > > > > > > > > > Thanks for the KIP Walker. > > > > > > > > > > > > > > > > > > > > +1 (binding) > > > > > > > > > > > > > > > > > > > > -Bill > > > > > > > > > > > > > > > > > > > > On Tue, Sep 29, 2020 at 4:59 PM Guozhang Wang < > > wangg...@gmail.com> > > > > > > > > wrote: > > > > > > > > > > > +1 again on the KIP. > > > > > > > > > > > > > > > > > > > > > > On Tue, Sep 29, 2020 at 1:51 PM Leah Thomas < > > ltho...@confluent.io > > > > > > > > wrote: > > > > > > > > > > > > Hey Walker, > > > > > > > > > > > > > > > > > > > > > > > > Thanks for the KIP! I'm +1, non-binding. > > > > > > > > > > > > > > > > > > > > > > > > Cheers, > > > > > > > > > > > > Leah > > > > > > > > > > > > > > > > > > > > > > > > On Tue, Sep 29, 2020 at 1:56 PM Walker Carlson < > > > > > > > > wcarl...@confluent.io> > > > > > > > > > > > > wrote: > > > > > > > > > > > > > > > > > > > > > > > > > Hello all, > > > > > > > > > > > > > > > > > > > > > > > > > > I made some changes to the KIP the descriptions are > > > > > > > > > > > > > on the > > > > > > > > discussion > > > > > > > > > > > > > thread. If you have already voted I would ask you to > > > > > > > > > > > > > confirm > > > > your > > > > > > > > vote. > > > > > > > > > > > > > Otherwise please vote so we can get this feature in. > > > > > > > > > > > > > > > > > > > > > > > > > > Thanks, > > > > > > > > > > > > > Walker > > > > > > > > > > > > > > > > > > > > > > > > > > On Thu, Sep 24, 2020 at 4:36 PM John Roesler < > > > > vvcep...@apache.org > > > > > > > > > > > > wrote: > > > > > > > > > > > > > > Thanks for the KIP, Walker! > > > > > > > > > > > > > > > > > > > > > > > > > > > > I’m +1 (binding) > > > > > > > > > > > > > > > > > > > > > > > > > > > > -John > > > > > > > > > > > > > > > > > > > > > > > > > > > > On Mon, Sep 21, 2020, at 17:04, Guozhang Wang wrote: > > > > > > > > > > > > > > > Thanks for finalizing the KIP. +1 (binding) > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > Guozhang > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > On Mon, Sep 21, 2020 at 1:38 PM Walker Carlson < > > > > > > > > > > > > wcarl...@confluent.io> > > > > > > > > > > > > > > > wrote: > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > Hello all, > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > I would like to start a thread to vote for > > > > > > > > > > > > > > > > KIP-671 to > > add a > > > > > > > > > > > method > > > > > > > > > > > > to > > > > > > > > > > > > > > close > > > > > > > > > > > > > > > > all clients in a kafka streams application. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > KIP: > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > https://cwiki.apache.org/confluence/display/KAFKA/KIP-671%3A+Shutdown+Streams+Application+when+appropriate+exception+is+thrown > > > > > > > > > > > > > > > > Discussion thread: *here > > > > > > > > > > > > > > > > < > > > > > > > > > > > > > > > > > > https://mail-archives.apache.org/mod_mbox/kafka-dev/202009.mbox/%3CCAC55fuh3HAGCxz-PzxTJraczy6T-os2oiCV328PBeuJQSVYASg%40mail.gmail.com%3E > > > > > > > > > > > > > > > > > * > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > Thanks, > > > > > > > > > > > > > > > > -Walker > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > -- > > > > > > > > > > > > > > > -- Guozhang > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > -- > > > > > > > > > > > -- Guozhang > > > > > > > > > > >
