KAFKA-20363 : server-common - Fix flaky testIdleTimeCallback in KafkaEventQueueTest#21920
KAFKA-20363 : server-common - Fix flaky testIdleTimeCallback in KafkaEventQueueTest#21920chia7712 merged 2 commits intoapache:trunkfrom
Conversation
github-actions
Bot
added
triage
|
Closing this pr @chia7712 . Sorry, didn't notice the interest comment from Kim. |
|
@muralibasani excuse me. Do we already have a PR for this flaky? |
|
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. |
|
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! 👍 |
| // 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, |
There was a problem hiding this comment.
Ah, I see the 'extra' indent is the existing convention in this file. I'll withdraw my comment
| // Test 2: Deferred event | ||
| // Wait for the queue thread to enter cond.await() before advancing MockTime. | ||
| TestUtils.waitForCondition( | ||
| () -> queueThread.getState() == Thread.State.WAITING, |
There was a problem hiding this comment.
@chia7712 shall I remove the extra indents ? Or it's ok to leave it as is ?
|
@muralibasani Could help to you re-trigger CI? |
muralibasani
force-pushed
the
KAFKA-20363
branch
from
9a20042 to
f6a8fab
Compare
muralibasani
changed the title
|
CI looks good. Ok to merge? |
|
@TaiJuWu any further review possible. Would be great. Thanks. |
|
|
||
| // Test 2: Deferred event | ||
| // Wait for the queue thread to enter cond.await() before advancing MockTime. | ||
| TestUtils.waitForCondition( |
There was a problem hiding this comment.
nit: Could we extract a helper method for this waiting?
There was a problem hiding this comment.
Indeed, there were 2 occurances. Thanks. Updated.
|
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. |
…ache#21920) https://issues.apache.org/jira/browse/KAFKA-20363 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. Reviewers: junvelop <151982401+junjunclub@users.noreply.github.com>, TaiJuWu <tjwu1217@gmail.com>, Chia-Ping Tsai <chia7712@gmail.com>
5 participants
https://issues.apache.org/jira/browse/KAFKA-20363
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.
Reviewers: junvelop 151982401+junjunclub@users.noreply.github.com,
TaiJuWu tjwu1217@gmail.com, Chia-Ping Tsai chia7712@gmail.com