> it's never a good sign when the discussion moves into the vote thread Hah, sorry, the gmail consolidation of [VOTE] and [DISCUSS] threads strikes again. Thanks for redirecting me Bruno
I suppose it's unfair to expect the callers to keep perfect track of the current number of stream threads, but it also seems like you shouldn't be calling removeStreamThread() when there are no threads left. Either you're just haphazardly removing threads and could unintentionally slip into a state of no running threads without realizing it, or more realistically, you're carefully removing threads based on some metric(s) that convey whether the system is over or under-provisioned. If your metrics say you're over-provisioned but there's not one thread running, well, that certainly sounds exceptional to me. Or you might be right in that the cluster is over-provisioned but have just been directing the removeStreamThread() and addStreamThread() calls to instances at random, and end up with one massive instance and one with no threads at all. Again, this probably merits some human intervention (or system redesign) That said, I don't think there's any real harm to just returning null in this case, but I hope that users would pay attention to this since it seems likely to indicate something has gone seriously wrong. I suppose Optional<String> would be a reasonable compromise. As for the method name, what about activeStreamThreads() or liveStreamThreads() ? On Mon, Sep 7, 2020 at 1:45 AM Bruno Cadonna <br...@confluent.io> wrote: > Hi John, > > I agree with you except for checking null. I would rather prefer to use > Optional<String> as the return type to both methods. > > I changed the subject from [VOTE] to [DISCUSS] so that we can follow up > in the discussion thread. > > 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 > > >
