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 >>>>>>>> >>>>>> >> >
