You are viewing a plain text version of this content. The canonical link for it is here.
Posted to jira@kafka.apache.org by "muralibasani (via GitHub)" <gi...@apache.org> on 2026/03/31 21:02:12 UTC
[PR] KAFKA-20363 : Fix flaky testIdleTimeCallback in KafkaEventQueueTest [kafka]
muralibasani opened a new pull request, #21920:
URL: https://github.com/apache/kafka/pull/21920
The test had a race condition between Test thread and Queue thread.
We actually wait until queue thread is sleeping/waiting so it guarantees the order.
--
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-20363 : Fix flaky testIdleTimeCallback in KafkaEventQueueTest [kafka]
Posted by "muralibasani (via GitHub)" <gi...@apache.org>.
muralibasani closed pull request #21920: KAFKA-20363 : Fix flaky testIdleTimeCallback in KafkaEventQueueTest
URL: https://github.com/apache/kafka/pull/21920
--
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-20363 : Fix flaky testIdleTimeCallback in KafkaEventQueueTest [kafka]
Posted by "chia7712 (via GitHub)" <gi...@apache.org>.
chia7712 commented on code in PR #21920:
URL: https://github.com/apache/kafka/pull/21920#discussion_r3026094082
##########
server-common/src/test/java/org/apache/kafka/queue/KafkaEventQueueTest.java:
##########
@@ -451,6 +456,13 @@ public void testIdleTimeCallback() throws Exception {
}));
assertEquals("event1-processed", event1.get());
+ // Wait for the queue thread to enter cond.await() before advancing MockTime.
+ // This ensures startIdleMs is captured before the time advancement.
+ TestUtils.waitForCondition(
+ () -> queueThread.getState() == Thread.State.WAITING,
Review Comment:
Ah, I see the 'extra' indent is the existing convention in this file. I'll withdraw my comment
--
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-20363 : server-common - Fix flaky testIdleTimeCallback in KafkaEventQueueTest [kafka]
Posted by "muralibasani (via GitHub)" <gi...@apache.org>.
muralibasani commented on code in PR #21920:
URL: https://github.com/apache/kafka/pull/21920#discussion_r3051628423
##########
server-common/src/test/java/org/apache/kafka/queue/KafkaEventQueueTest.java:
##########
@@ -464,6 +476,12 @@ public void testIdleTimeCallback() throws Exception {
assertEquals(timeBeforeWait + waitTime5Ms, lastCurrentTimeMs.get(), "Current time should be " + (timeBeforeWait + waitTime5Ms) + "ms, was: " + lastCurrentTimeMs.get());
// Test 2: Deferred event
+ // Wait for the queue thread to enter cond.await() before advancing MockTime.
+ TestUtils.waitForCondition(
Review Comment:
Indeed, there were 2 occurances. Thanks. Updated.
--
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-20363 : Fix flaky testIdleTimeCallback in KafkaEventQueueTest [kafka]
Posted by "junjunclub (via GitHub)" <gi...@apache.org>.
junjunclub commented on PR #21920:
URL: https://github.com/apache/kafka/pull/21920#issuecomment-4170167313
LGTM! The race condition analysis is spot on — waiting for the queue thread to reach WAITING state before advancing MockTime ensures startIdleMs is captured correctly. I was actually thinking the exact same thing. Nice fix! 👍
--
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-20363 : server-common - Fix flaky testIdleTimeCallback in KafkaEventQueueTest [kafka]
Posted by "muralibasani (via GitHub)" <gi...@apache.org>.
muralibasani commented on PR #21920:
URL: https://github.com/apache/kafka/pull/21920#issuecomment-4206441067
@TaiJuWu any further review possible. Would be great. 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-20363 : server-common - Fix flaky testIdleTimeCallback in KafkaEventQueueTest [kafka]
Posted by "chia7712 (via GitHub)" <gi...@apache.org>.
chia7712 commented on PR #21920:
URL: https://github.com/apache/kafka/pull/21920#issuecomment-4222183328
> Or because of code freeze, we must wait probably
Only the 4.3 branch is frozen; the door to trunk is still open! :)
--
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-20363 : server-common - Fix flaky testIdleTimeCallback in KafkaEventQueueTest [kafka]
Posted by "chia7712 (via GitHub)" <gi...@apache.org>.
chia7712 merged PR #21920:
URL: https://github.com/apache/kafka/pull/21920
--
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-20363 : Fix flaky testIdleTimeCallback in KafkaEventQueueTest [kafka]
Posted by "muralibasani (via GitHub)" <gi...@apache.org>.
muralibasani commented on code in PR #21920:
URL: https://github.com/apache/kafka/pull/21920#discussion_r3026470642
##########
server-common/src/test/java/org/apache/kafka/queue/KafkaEventQueueTest.java:
##########
@@ -464,6 +476,12 @@ public void testIdleTimeCallback() throws Exception {
assertEquals(timeBeforeWait + waitTime5Ms, lastCurrentTimeMs.get(), "Current time should be " + (timeBeforeWait + waitTime5Ms) + "ms, was: " + lastCurrentTimeMs.get());
// Test 2: Deferred event
+ // Wait for the queue thread to enter cond.await() before advancing MockTime.
+ TestUtils.waitForCondition(
+ () -> queueThread.getState() == Thread.State.WAITING,
Review Comment:
@chia7712 shall I remove the extra indents ? Or it's ok to leave it as is ?
--
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-20363 : Fix flaky testIdleTimeCallback in KafkaEventQueueTest [kafka]
Posted by "chia7712 (via GitHub)" <gi...@apache.org>.
chia7712 commented on PR #21920:
URL: https://github.com/apache/kafka/pull/21920#issuecomment-4169019360
@muralibasani excuse me. Do we already have a PR for this flaky?
--
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-20363 : server-common - Fix flaky testIdleTimeCallback in KafkaEventQueueTest [kafka]
Posted by "muralibasani (via GitHub)" <gi...@apache.org>.
muralibasani commented on PR #21920:
URL: https://github.com/apache/kafka/pull/21920#issuecomment-4222217641
> > Or because of code freeze, we must wait probably
>
> Only the 4.3 branch is frozen; the door to trunk is still open! :)
Nice. So my first pr on 4.4.0.
--
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-20363 : Fix flaky testIdleTimeCallback in KafkaEventQueueTest [kafka]
Posted by "TaiJuWu (via GitHub)" <gi...@apache.org>.
TaiJuWu commented on PR #21920:
URL: https://github.com/apache/kafka/pull/21920#issuecomment-4181298209
@muralibasani Could help to you re-trigger CI?
--
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-20363 : Fix flaky testIdleTimeCallback in KafkaEventQueueTest [kafka]
Posted by "chia7712 (via GitHub)" <gi...@apache.org>.
chia7712 commented on code in PR #21920:
URL: https://github.com/apache/kafka/pull/21920#discussion_r3026062114
##########
server-common/src/test/java/org/apache/kafka/queue/KafkaEventQueueTest.java:
##########
@@ -451,6 +456,13 @@ public void testIdleTimeCallback() throws Exception {
}));
assertEquals("event1-processed", event1.get());
+ // Wait for the queue thread to enter cond.await() before advancing MockTime.
+ // This ensures startIdleMs is captured before the time advancement.
+ TestUtils.waitForCondition(
+ () -> queueThread.getState() == Thread.State.WAITING,
Review Comment:
Please remove extra indent
##########
server-common/src/test/java/org/apache/kafka/queue/KafkaEventQueueTest.java:
##########
@@ -464,6 +476,12 @@ public void testIdleTimeCallback() throws Exception {
assertEquals(timeBeforeWait + waitTime5Ms, lastCurrentTimeMs.get(), "Current time should be " + (timeBeforeWait + waitTime5Ms) + "ms, was: " + lastCurrentTimeMs.get());
// Test 2: Deferred event
+ // Wait for the queue thread to enter cond.await() before advancing MockTime.
+ TestUtils.waitForCondition(
+ () -> queueThread.getState() == Thread.State.WAITING,
Review Comment:
ditto
--
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-20363 : Fix flaky testIdleTimeCallback in KafkaEventQueueTest [kafka]
Posted by "chia7712 (via GitHub)" <gi...@apache.org>.
chia7712 commented on code in PR #21920:
URL: https://github.com/apache/kafka/pull/21920#discussion_r3026615589
##########
server-common/src/test/java/org/apache/kafka/queue/KafkaEventQueueTest.java:
##########
@@ -464,6 +476,12 @@ public void testIdleTimeCallback() throws Exception {
assertEquals(timeBeforeWait + waitTime5Ms, lastCurrentTimeMs.get(), "Current time should be " + (timeBeforeWait + waitTime5Ms) + "ms, was: " + lastCurrentTimeMs.get());
// Test 2: Deferred event
+ // Wait for the queue thread to enter cond.await() before advancing MockTime.
+ TestUtils.waitForCondition(
+ () -> queueThread.getState() == Thread.State.WAITING,
Review Comment:
It is ok to leave it as is :)
--
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-20363 : server-common - Fix flaky testIdleTimeCallback in KafkaEventQueueTest [kafka]
Posted by "chia7712 (via GitHub)" <gi...@apache.org>.
chia7712 commented on code in PR #21920:
URL: https://github.com/apache/kafka/pull/21920#discussion_r3051559566
##########
server-common/src/test/java/org/apache/kafka/queue/KafkaEventQueueTest.java:
##########
@@ -464,6 +476,12 @@ public void testIdleTimeCallback() throws Exception {
assertEquals(timeBeforeWait + waitTime5Ms, lastCurrentTimeMs.get(), "Current time should be " + (timeBeforeWait + waitTime5Ms) + "ms, was: " + lastCurrentTimeMs.get());
// Test 2: Deferred event
+ // Wait for the queue thread to enter cond.await() before advancing MockTime.
+ TestUtils.waitForCondition(
Review Comment:
nit: Could we extract a helper method for this waiting?
--
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-20363 : server-common - Fix flaky testIdleTimeCallback in KafkaEventQueueTest [kafka]
Posted by "muralibasani (via GitHub)" <gi...@apache.org>.
muralibasani commented on PR #21920:
URL: https://github.com/apache/kafka/pull/21920#issuecomment-4222144981
Or because of code freeze, we must wait probably
--
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-20363 : Fix flaky testIdleTimeCallback in KafkaEventQueueTest [kafka]
Posted by "muralibasani (via GitHub)" <gi...@apache.org>.
muralibasani commented on PR #21920:
URL: https://github.com/apache/kafka/pull/21920#issuecomment-4165573645
Closing this pr @chia7712 . Sorry, didn't notice the interest comment from Kim.
--
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-20363 : Fix flaky testIdleTimeCallback in KafkaEventQueueTest [kafka]
Posted by "chia7712 (via GitHub)" <gi...@apache.org>.
chia7712 commented on PR #21920:
URL: https://github.com/apache/kafka/pull/21920#issuecomment-4169030073
I've seen the comments. Let's move forward with your PR. Even if there's another similar proposal, I don't see it as a problem.
--
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-20363 : Fix flaky testIdleTimeCallback in KafkaEventQueueTest [kafka]
Posted by "muralibasani (via GitHub)" <gi...@apache.org>.
muralibasani commented on PR #21920:
URL: https://github.com/apache/kafka/pull/21920#issuecomment-4169134914
> I've seen the comments. Let's move forward with your PR. Even if there's another similar proposal, I don't see it as a problem.
@chia7712 thankyou. Re-opened this pr again.
--
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-20363 : server-common - Fix flaky testIdleTimeCallback in KafkaEventQueueTest [kafka]
Posted by "muralibasani (via GitHub)" <gi...@apache.org>.
muralibasani commented on PR #21920:
URL: https://github.com/apache/kafka/pull/21920#issuecomment-4188568774
CI looks good. Ok to merge?
--
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-20363 : server-common - Fix flaky testIdleTimeCallback in KafkaEventQueueTest [kafka]
Posted by "muralibasani (via GitHub)" <gi...@apache.org>.
muralibasani commented on PR #21920:
URL: https://github.com/apache/kafka/pull/21920#issuecomment-4222010956
@chia7712 @TaiJuWu Hope one last review could be good for this. Thank you.
--
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