divijvaidya commented on code in PR #12465:
URL: https://github.com/apache/kafka/pull/12465#discussion_r997374051
##########
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:
Thank you both for the discussion here.
I believe that we cannot use `streamThread.state().isAlive()` here because a
thread in `CREATED` state is not considered `isAlive()` by this method which
can lead to the following regression from existing behaviour:
1. Inside `addStreamThread()`, thread gets created and added to `threads`
list at `createAndAddStreamThread()` but Thread.start() (which transitions the
thread state to STARTING) hasn't been called yet (because, let's say current
thread is waiting at line 1033 in addStreamThread() method).
2. `removeStreamThread` gets called by another thread.
**Old behaviour:**
`streamThread.isAlive()` will return true and the thread will be removed
from the list. When the first thread
**New behaviour:**
`streamThread.state().isAlive()` will return false and the thread will not
be removed from the list, hence `removeStreamThread` will not remove this
thread.
I believe that this new behaviour is a change in public interface contract
for `removeStreamThread` and hence, perhaps could be a breaking change.
Alternatively,
the simplest solution is to create a new method which is a wrapper over the
native method such as:
```
// Ensure Mockito can stub method for KafkaStreamTest.
public boolean isStateAlive() {
return state.isAlive();
}
```
Is that something that would be acceptable to you folks?
--
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]
