cadonna commented on code in PR #12465:
URL: https://github.com/apache/kafka/pull/12465#discussion_r984479081
##########
streams/src/main/java/org/apache/kafka/streams/KafkaStreams.java:
##########
@@ -1096,7 +1096,7 @@ private Optional<String> removeStreamThread(final long
timeoutMs) throws Timeout
// make a copy of threads to avoid holding lock
for (final StreamThread streamThread : new
ArrayList<>(threads)) {
final boolean callingThreadIsNotCurrentStreamThread =
!streamThread.getName().equals(Thread.currentThread().getName());
- if (streamThread.isAlive() &&
(callingThreadIsNotCurrentStreamThread || getNumLiveStreamThreads() == 1)) {
+ if (callingThreadIsNotCurrentStreamThread ||
getNumLiveStreamThreads() == 1) {
Review Comment:
@wcarlson5 Could you please help me understanding the implications of
removing this check?
My take is that:
Without the check:
- If the stream thread is not alive and its state is not `DEAD`, it will not
be removed from the list of threads.
- If the stream thread is not alive and its state is `DEAD`, it will be
removed from the list of threads.
- If the stream thread is alive and its state is not `DEAD`, it will be
removed from the list of threads once it reaches `DEAD` -- here, in
`getNumLiveStreamThreads()`, or in `getNextThreadIndex()`.
- If the stream thread is alive and its state is `DEAD`, it will be removed
from the list of threads.
With the check:
- If the stream thread is not alive and its state is not `DEAD`, it will not
be removed from the list of threads.
- If the stream thread is not alive and its state is `DEAD`, it will be
removed from the list of threads in `getNumLiveStreamThreads()` or in
`getNextThreadIndex()` but not here.
- If the stream thread is alive and its state is not `DEAD`, it will be
removed from the list of threads once it reaches `DEAD` -- here, in
`getNumLiveStreamThreads()`, or in `getNextThreadIndex()`.
- If the stream thread is alive and its state is `DEAD`, it will be removed
from the list of threads.
So it seems the check does not change much except that it delays the removal
from the list if the stream thread is not alive and its state is `DEAD`.
Did I miss a case?
For context, this check blocks the migration to Mockito since Mockito cannot
stub native methods and `isAlive()` is a native method.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
