You are viewing a plain text version of this content. The canonical link for it is here.
Posted to jira@kafka.apache.org by "apalan60 (via GitHub)" <gi...@apache.org> on 2025/05/03 06:30:49 UTC
[PR] KAFKA-18537: Fix flaky RemoteIndexCacheTest#testCleanerThreadShutdown [kafka]
apalan60 opened a new pull request, #19628:
URL: https://github.com/apache/kafka/pull/19628
Add a wait for cleaner thread shutdown in `testCleanerThreadShutdown` to eliminate flakiness. After calling `cache.close()`, the test now uses `TestUtils.waitForCondition` to poll until the background “remote-log-index-cleaner” thread has fully exited before asserting that no cleaner threads remain. This ensures the asynchronous shutdown always completes before the final assertions.
--
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-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-18537: Fix flaky RemoteIndexCacheTest#testCleanerThreadShutdown [kafka]
Posted by "chia7712 (via GitHub)" <gi...@apache.org>.
chia7712 commented on code in PR #19628:
URL: https://github.com/apache/kafka/pull/19628#discussion_r2073346146
##########
storage/src/test/java/org/apache/kafka/storage/internals/log/RemoteIndexCacheTest.java:
##########
@@ -376,8 +376,13 @@ public void testCleanerThreadShutdown() throws IOException, InterruptedException
// close the cache properly
cache.close();
// verify that the thread is closed properly
- threads = getRunningCleanerThread();
- assertTrue(threads.isEmpty(), "Found unexpected " + threads.size() + " threads=" + threads.stream().map(Thread::getName).collect(Collectors.joining(", ")));
+ TestUtils.waitForCondition(
Review Comment:
```java
TestUtils.waitForCondition(
() -> getRunningCleanerThread().isEmpty(),
() -> "Failed while waiting for cleaner threads to shutdown. Remaining threads: " +
getRunningCleanerThread().stream().map(Thread::getName).collect(Collectors.joining(", ")));
```
--
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-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-18537: Fix flaky RemoteIndexCacheTest#testCleanerThreadShutdown [kafka]
Posted by "apalan60 (via GitHub)" <gi...@apache.org>.
apalan60 commented on code in PR #19628:
URL: https://github.com/apache/kafka/pull/19628#discussion_r2073367271
##########
storage/src/test/java/org/apache/kafka/storage/internals/log/RemoteIndexCacheTest.java:
##########
@@ -376,8 +376,13 @@ public void testCleanerThreadShutdown() throws IOException, InterruptedException
// close the cache properly
cache.close();
// verify that the thread is closed properly
- threads = getRunningCleanerThread();
- assertTrue(threads.isEmpty(), "Found unexpected " + threads.size() + " threads=" + threads.stream().map(Thread::getName).collect(Collectors.joining(", ")));
+ TestUtils.waitForCondition(
Review Comment:
Updated. PTAL, thanks!
--
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-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-18537: Fix flaky RemoteIndexCacheTest#testCleanerThreadShutdown [kafka]
Posted by "apalan60 (via GitHub)" <gi...@apache.org>.
apalan60 commented on PR #19628:
URL: https://github.com/apache/kafka/pull/19628#issuecomment-2848465207
I reproduced the original failure by looping the full `RemoteIndexCacheTest` 100 times.
Before this change the test would intermittently fail within those 100 runs due to the cleaner thread still running. After applying the fix, I ran the same 100-iteration loop and saw zero failures, confirming the race condition is resolved.
--
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-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-18537: Fix flaky RemoteIndexCacheTest#testCleanerThreadShutdown [kafka]
Posted by "chia7712 (via GitHub)" <gi...@apache.org>.
chia7712 commented on code in PR #19628:
URL: https://github.com/apache/kafka/pull/19628#discussion_r2072957378
##########
storage/src/test/java/org/apache/kafka/storage/internals/log/RemoteIndexCacheTest.java:
##########
@@ -376,6 +376,8 @@ public void testCleanerThreadShutdown() throws IOException, InterruptedException
// close the cache properly
cache.close();
// verify that the thread is closed properly
+ TestUtils.waitForCondition(() -> getRunningCleanerThread().isEmpty(),
Review Comment:
this new check is duplicate to existent check. Could you please remove old check?
--
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-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-18537: Fix flaky RemoteIndexCacheTest#testCleanerThreadShutdown [kafka]
Posted by "apalan60 (via GitHub)" <gi...@apache.org>.
apalan60 commented on code in PR #19628:
URL: https://github.com/apache/kafka/pull/19628#discussion_r2073340999
##########
storage/src/test/java/org/apache/kafka/storage/internals/log/RemoteIndexCacheTest.java:
##########
@@ -376,6 +376,8 @@ public void testCleanerThreadShutdown() throws IOException, InterruptedException
// close the cache properly
cache.close();
// verify that the thread is closed properly
+ TestUtils.waitForCondition(() -> getRunningCleanerThread().isEmpty(),
Review Comment:
@chia7712
Thanks for your review. I’ve removed the old check and reused the existing logic to maintain consistent behavior. PTAL.
--
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-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-18537: Fix flaky RemoteIndexCacheTest#testCleanerThreadShutdown [kafka]
Posted by "apalan60 (via GitHub)" <gi...@apache.org>.
apalan60 commented on code in PR #19628:
URL: https://github.com/apache/kafka/pull/19628#discussion_r2072612894
##########
storage/src/test/java/org/apache/kafka/storage/internals/log/RemoteIndexCacheTest.java:
##########
@@ -376,6 +376,8 @@ public void testCleanerThreadShutdown() throws IOException, InterruptedException
// close the cache properly
cache.close();
// verify that the thread is closed properly
+ TestUtils.waitForCondition(() -> getRunningCleanerThread().isEmpty(),
+ "Failed while waiting for cleaner thread to shutdown");
Review Comment:
@frankvicky
Thanks for your review. I've updated it.
--
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-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-18537: Fix flaky RemoteIndexCacheTest#testCleanerThreadShutdown [kafka]
Posted by "chia7712 (via GitHub)" <gi...@apache.org>.
chia7712 merged PR #19628:
URL: https://github.com/apache/kafka/pull/19628
--
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-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-18537: Fix flaky RemoteIndexCacheTest#testCleanerThreadShutdown [kafka]
Posted by "frankvicky (via GitHub)" <gi...@apache.org>.
frankvicky commented on code in PR #19628:
URL: https://github.com/apache/kafka/pull/19628#discussion_r2072535264
##########
storage/src/test/java/org/apache/kafka/storage/internals/log/RemoteIndexCacheTest.java:
##########
@@ -376,6 +376,8 @@ public void testCleanerThreadShutdown() throws IOException, InterruptedException
// close the cache properly
cache.close();
// verify that the thread is closed properly
+ TestUtils.waitForCondition(() -> getRunningCleanerThread().isEmpty(),
+ "Failed while waiting for cleaner thread to shutdown");
Review Comment:
```suggestion
"Failed while waiting for cleaner threads to shutdown");
```
--
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-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org