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