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