+1
On 10/19/20 12:33 PM, John Roesler wrote: > 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 >>>>>>>>>>>>>> >
