Thanks for clarifying that @Sophie it is in regards to KIP-633 On Wed, Jun 16, 2021 at 4:00 PM Sophie Blee-Goldman <sop...@confluent.io.invalid> wrote:
> Matthias, I'm guessing you meant to send this to the KIP-633 list? This is > KIP-663 > > On Wed, Jun 16, 2021 at 12:37 PM Matthias J. Sax <mj...@apache.org> wrote: > > > Quick follow up. I did a small update to the KIP with regard to > > https://issues.apache.org/jira/browse/KAFKA-12909 > > > > Israel, Sophie, and Guozhang did agree to this change. I don't think we > > need to re-vote. > > > > Please let us know if there are any concerns. > > > > > > -Matthias > > > > On 1/27/21 12:48 PM, Sophie Blee-Goldman wrote: > > > 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 > > >>>>>>>> > > >>>>>> > > >> > > > > > >
