Thanks, Bruno, this sounds good to me. -John
On Tue, Sep 29, 2020, at 03:13, Bruno Cadonna wrote: > Hi all, > > I did two minor modifications to the KIP. > > - I removed the rather strict guarantee "Dead stream threads are removed > from a Kafka Streams client at latest after the next call to > KafkaStreams#addStreamThread() or KafkaStreams#removeStreamThread() > following the transition to state DEAD." > Dead stream threads will be still removed, but the behavior will be less > strict. > > - Added a sentence that states that the Kafka Streams client will > transit to ERROR if the last alive stream thread dies exceptionally. > This corresponds to the current behavior. > > I will not restart voting and keep the votes so far. > > Best, > Bruno > > On 22.09.20 01:19, John Roesler wrote: > > I’m +1 also. Thanks, Bruno! > > -John > > > > On Mon, Sep 21, 2020, at 17:08, Guozhang Wang wrote: > >> Thanks Bruno. I'm +1 on the KIP. > >> > >> On Mon, Sep 21, 2020 at 2:49 AM Bruno Cadonna <br...@confluent.io> wrote: > >> > >>> Hi, > >>> > >>> I would like to restart from zero the voting on KIP-663 that proposes to > >>> add methods to the Kafka Streams client to add and remove stream threads > >>> during execution. > >>> > >>> > >>> https://cwiki.apache.org/confluence/display/KAFKA/KIP-663%3A+API+to+Start+and+Shut+Down+Stream+Threads > >>> > >>> Matthias, if you are still +1, please vote again. > >>> > >>> Best, > >>> Bruno > >>> > >>> On 04.09.20 23:12, John Roesler wrote: > >>>> Hi Sophie, > >>>> > >>>> Uh, oh, it's never a good sign when the discussion moves > >>>> into the vote thread :) > >>>> > >>>> I agree with you, it seems like a good touch for > >>>> removeStreamThread() to return the name of the thread that > >>>> got removed, rather than a boolean flag. Maybe the return > >>>> value would be `null` if there is no thread to remove. > >>>> > >>>> If we go that way, I'd suggest that addStreamThread() also > >>>> return the name of the newly created thread, or null if no > >>>> thread can be created right now. > >>>> > >>>> I'm not completely sure if I think that callers of this > >>>> method would know exactly how many threads there are. Sure, > >>>> if a human being is sitting there looking at the metrics or > >>>> logs and decides to call the method, it would work out, but > >>>> I'd expect this kind of method to find its way into > >>>> automated tooling that reacts to things like current system > >>>> load or resource saturation. Those kinds of toolchains often > >>>> are part of a distributed system, and it's probably not that > >>>> easy to guarantee that the thread count they observe is > >>>> fully consistent with the number of threads that are > >>>> actually running. Therefore, an in-situ `int > >>>> numStreamThreads()` method might not be a bad idea. Then > >>>> again, it seems sort of optional. A caller can catch an > >>>> exception or react to a `null` return value just the same > >>>> either way. Having both add/remove methods behave similarly > >>>> is probably more valuable. > >>>> > >>>> Thanks, > >>>> -John > >>>> > >>>> > >>>> On Thu, 2020-09-03 at 12:15 -0700, Sophie Blee-Goldman > >>>> wrote: > >>>>> Hey, sorry for the late reply, I just have one minor suggestion. Since > >>> we > >>>>> don't > >>>>> make any guarantees about which thread gets removed or allow the user to > >>>>> specify, I think we should return either the index or full name of the > >>>>> thread > >>>>> that does get removed by removeThread(). > >>>>> > >>>>> I know you just updated the KIP to return true/false if there > >>> are/aren't any > >>>>> threads to be removed, but I think this would be more appropriate as an > >>>>> exception than as a return type. I think it's reasonable to expect > >>> users to > >>>>> have some sense to how many threads are remaining, and not try to remove > >>>>> a thread when there is none left. To me, that indicates something wrong > >>>>> with the user application code and should be treated as an exceptional > >>> case. > >>>>> I don't think the same code clarify argument applies here as to the > >>>>> addStreamThread() case, as there's no reason for an application to be > >>>>> looping and retrying removeStreamThread() since if that fails, it's > >>> because > >>>>> there are no threads left and thus it will continue to always fail. And > >>> if > >>>>> the > >>>>> user actually wants to shut down all threads, they should just close the > >>>>> whole application rather than call removeStreamThread() in a loop. > >>>>> > >>>>> While I generally think it should be straightforward for users to track > >>> how > >>>>> many stream threads they have running, maybe it would be nice to add > >>>>> a small utility method that does this for them. Something like > >>>>> > >>>>> // Returns the number of currently alive threads > >>>>> boolean runningStreamThreads(); > >>>>> > >>>>> On Thu, Sep 3, 2020 at 7:41 AM Matthias J. Sax <mj...@apache.org> > >>> wrote: > >>>>> > >>>>>> +1 (binding) > >>>>>> > >>>>>> On 9/3/20 6:16 AM, Bruno Cadonna wrote: > >>>>>>> Hi, > >>>>>>> > >>>>>>> I would like to start the voting on KIP-663 that proposes to add > >>> methods > >>>>>>> to the Kafka Streams client to add and remove stream threads during > >>>>>>> execution. > >>>>>>> > >>>>>>> > >>>>>> > >>> https://cwiki.apache.org/confluence/display/KAFKA/KIP-663%3A+API+to+Start+and+Shut+Down+Stream+Threads > >>>>>>> > >>>>>>> Best, > >>>>>>> Bruno > >>>> > >>> > >> > >> > >> -- > >> -- Guozhang > >> >
