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