[GitHub] [kafka] wcarlson5 commented on a diff in pull request #12465: KAFKA-12950: Replace EasyMock and PowerMock with Mockito for KafkaStreamsTest
wcarlson5 commented on code in PR #12465:
URL: https://github.com/apache/kafka/pull/12465#discussion_r995860323
##
streams/src/main/java/org/apache/kafka/streams/KafkaStreams.java:
##
@@ -1096,7 +1096,7 @@ private Optional 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:
Maybe, if the mock works then proabably
--
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: jira-unsubscr...@kafka.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [kafka] wcarlson5 commented on a diff in pull request #12465: KAFKA-12950: Replace EasyMock and PowerMock with Mockito for KafkaStreamsTest
wcarlson5 commented on code in PR #12465:
URL: https://github.com/apache/kafka/pull/12465#discussion_r995013622
##
streams/src/main/java/org/apache/kafka/streams/KafkaStreams.java:
##
@@ -1096,7 +1096,7 @@ private Optional 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:
You can look at the is alive method for state checks that are happening and
refactor it into a method that works for Mockito.
Ah that makes more sense. Thanks for clarifying
--
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: jira-unsubscr...@kafka.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [kafka] wcarlson5 commented on a diff in pull request #12465: KAFKA-12950: Replace EasyMock and PowerMock with Mockito for KafkaStreamsTest
wcarlson5 commented on code in PR #12465:
URL: https://github.com/apache/kafka/pull/12465#discussion_r990331318
##
streams/src/main/java/org/apache/kafka/streams/KafkaStreams.java:
##
@@ -1096,7 +1096,7 @@ private Optional 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:
So this check is important and I don't think we can just remove it.
The important thing this is doing is preventing a thread from being removed
that is already dead. If a thread is removed but not finished cleaning up it is
still in the thread list. If we then call remove threads and that dead thread
is picked then no thread will really be removed so that call is broken. Even
worse though the cache size of all the other threads are changed to some value
that is not correct.
Instead of just removing the check you can maybe change to do something like
`thread.state() == StreamThread.State.DEAD`
Maybe you can add a helper method to check that instead of using the native
methods if w really need to.
On a side note it is concerning to me that we can't mock native methods
anymore that does not seem a good thing. Why are we removing this ablity?
--
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: jira-unsubscr...@kafka.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
