showuon commented on code in PR #15678:
URL: https://github.com/apache/kafka/pull/15678#discussion_r1560863382
##########
server/src/test/java/org/apache/kafka/server/AssignmentsManagerTest.java:
##########
@@ -172,10 +172,11 @@ public void testAssignmentAggregation() throws
InterruptedException {
manager.onAssignment(new TopicIdPartition(TOPIC_1, 3), DIR_3, () -> {
});
manager.onAssignment(new TopicIdPartition(TOPIC_1, 4), DIR_1, () -> {
});
manager.onAssignment(new TopicIdPartition(TOPIC_2, 5), DIR_2, () -> {
});
- while (!readyToAssert.await(1, TimeUnit.MILLISECONDS)) {
+ TestUtils.waitForCondition(() -> {
time.sleep(100);
manager.wakeup();
- }
+ return readyToAssert.await(1, TimeUnit.MILLISECONDS);
+ }, 1000, "Wait for ready to assert.");
Review Comment:
1. The current error message will output like this when failed:
`Condition not met within timeout 1000. Wait for ready to assert.`
([ref](https://github.com/apache/kafka/blob/trunk/clients/src/test/java/org/apache/kafka/test/TestUtils.java#L397))
I think we should update the error message as: `Timed out waiting for
AssignReplicasToDirsRequest to be sent.` to make it clear and meaningful.
2. The timeout 1000ms (1 sec) is too short in CI env. We can use the default
timeout value (15 secs) directly without providing the parameter. Ex:
```
TestUtils.waitForCondition(() -> {
time.sleep(1000);
...
}, "Wait for ready to assert");
```
Same comments apply to below changes.
--
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
