-
Notifications
You must be signed in to change notification settings - Fork 14.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
KAFAK-18451: Flaky RemoteLogManagerTest#testRLMOpsWhenMetadataIsNotReady #18520
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, a minor comment left. Thanks for the fix!
doReturn(true).when(remoteLogMetadataManager).isReady(any(TopicIdPartition.class)); | ||
|
||
CountDownLatch latch = new CountDownLatch(3); // there are 3 RLMTasks, so setting the count to 3 | ||
when(remoteLogMetadataManager.isReady(any(TopicIdPartition.class))) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think L3718 is not needed because we want to use the mock in L3721, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I just copied and pasted setup
function and didn't notice that. Thanks for catching it.
}; | ||
doReturn(true).when(remoteLogMetadataManager).isReady(any(TopicIdPartition.class)); | ||
|
||
CountDownLatch latch = new CountDownLatch(3); // there are 3 RLMTasks, so setting the count to 3 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice catch!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
Signed-off-by: PoAn Yang <[email protected]>
Signed-off-by: PoAn Yang <[email protected]>
84e5a8a
to
17102e1
Compare
Re-running CI. |
Hi @showuon, thanks for trigger CI again. There are some flaky test cases which are not related to this PR. I can't reproduce on my laptop as well.
|
…ady (apache#18520) The REMOTE_LOG_MANAGER_TASK_INTERVAL_MS_PROP in RemoteLogManagerTest is 100 which is too small. If assertions verifyNoMoreInteractions can't run in 100ms, the scheduler will run RLMTask again and the case will fail. Reviewers: Luke Chen <[email protected]>
…ady (apache#18520) The REMOTE_LOG_MANAGER_TASK_INTERVAL_MS_PROP in RemoteLogManagerTest is 100 which is too small. If assertions verifyNoMoreInteractions can't run in 100ms, the scheduler will run RLMTask again and the case will fail. Reviewers: Luke Chen <[email protected]>
…ady (apache#18520) The REMOTE_LOG_MANAGER_TASK_INTERVAL_MS_PROP in RemoteLogManagerTest is 100 which is too small. If assertions verifyNoMoreInteractions can't run in 100ms, the scheduler will run RLMTask again and the case will fail. Reviewers: Luke Chen <[email protected]>
The
REMOTE_LOG_MANAGER_TASK_INTERVAL_MS_PROP
inRemoteLogManagerTest
is 100 which is too small. If assertionsverifyNoMoreInteractions
can't run in 100ms, the scheduler will runRLMTask
again and the case will fail.Committer Checklist (excluded from commit message)