Thanks, Walker. That change looks good to me.
-John On Mon, 2020-10-19 at 12:06 -0700, Walker Carlson wrote: > Hello all, > > Taking into account the feedback about that last change I have removed some > of the changes and no longer will we have a separate handler for the global > thread. To make it so we can align the handlers there will also be no > option to just remove a stream thread. > > https://cwiki.apache.org/confluence/display/KAFKA/KIP-671%3A+Introduce+Kafka+Streams+Specific+Uncaught+Exception+Handler > > If you have any concerns please let me know, > Walker > > On Wed, Oct 14, 2020 at 12:51 PM John Roesler <vvcep...@apache.org> wrote: > > > 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 > > > > > > > > > > > > >
