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