Thanks Bruno, that sounds like a good addition. +1 On Wed, Jan 27, 2021 at 12:34 PM Bruno Cadonna <br...@confluent.io> wrote:
> Hi all, > > During the implementation, we notices that method removeStreamThread() > may block indefinitely when the stream thread chosen for removal is > blocked and cannot be shut down. Thus, we will add an overload that > takes a timeout. The newly added method will throw a TimeoutException, > when the timeout is exceeded. > > We updated the KIP accordingly. > > KIP: https://cwiki.apache.org/confluence/x/FDd4CQ > > Best, > Bruno > > On 30.09.20 13:51, Bruno Cadonna wrote: > > Thank you all for voting! > > > > This KIP is accepted with 3 binding +1 (Guozhang, John, Matthias). > > > > Best, > > Bruno > > > > On 29.09.20 22:24, Matthias J. Sax wrote: > >> +1 (binding) > >> > >> I am not super happy with the impact on the client state. For example, I > >> don't understand why it's ok to scale out if we lose one thread out of > >> four, but why it's not ok to scale out if we lose one thread out of one > >> (for this case, we would enter ERROR state and cannot add new threads > >> afterwards). > >> > >> However, this might be an issue for a follow up KIP. > >> > >> > >> -Matthias > >> > >> On 9/29/20 7:20 AM, John Roesler wrote: > >>> 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 > >>>>>> > >>>> >
