Re: [PR] MINOR : Replaced the while loop with TestUtils.waitForCondition [kafka]
chia7712 merged PR #15678: URL: https://github.com/apache/kafka/pull/15678 -- 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
Re: [PR] MINOR : Replaced the while loop with TestUtils.waitForCondition [kafka]
chiacyu commented on PR #15678: URL: https://github.com/apache/kafka/pull/15678#issuecomment-2055793708 Thanks for the comments. Just applied. -- 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
Re: [PR] MINOR : Replaced the while loop with TestUtils.waitForCondition [kafka]
showuon commented on PR #15678:
URL: https://github.com/apache/kafka/pull/15678#issuecomment-2054458671
@chiacyu , thanks for the update. LGTM! Comments:
1. Please resolve merge conflict
2. In L163, we did:
```
if (readyToAssert.getCount() > 0) {
readyToAssert.countDown();
}
```
Actually we can remove the if condition because the javadoc of
CountDownLatch#countDown said:
> If the current count equals zero then nothing happens.
Same comment applies to other similar places.
--
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
Re: [PR] MINOR : Replaced the while loop with TestUtils.waitForCondition [kafka]
chiacyu commented on code in PR #15678:
URL: https://github.com/apache/kafka/pull/15678#discussion_r1562574967
##
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:
Great comments, just applied. 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-unsubscr...@kafka.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] MINOR : Replaced the while loop with TestUtils.waitForCondition [kafka]
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
Re: [PR] MINOR : Replaced the while loop with TestUtils.waitForCondition [kafka]
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 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
Re: [PR] MINOR : Replaced the while loop with TestUtils.waitForCondition [kafka]
chiacyu commented on PR #15678: URL: https://github.com/apache/kafka/pull/15678#issuecomment-2049461096 Thanks for the reminder, let me check on that. -- 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
Re: [PR] MINOR : Replaced the while loop with TestUtils.waitForCondition [kafka]
chia7712 commented on PR #15678: URL: https://github.com/apache/kafka/pull/15678#issuecomment-2049107538 @chiacyu it seems `AssignmentsManagerTest` become unstable. could you check them? -- 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
Re: [PR] MINOR : Replaced the while loop with TestUtils.waitForCondition [kafka]
chia7712 commented on PR #15678: URL: https://github.com/apache/kafka/pull/15678#issuecomment-2047578617 > If the condition doesn't meet within the maxWaitMs, the waitForCondition would throw the assertion failure. Do we need another timeout to handle that? My origin thought was to use Junit 5 timeout, but we can't observe the method from the error stack if the error is produced by junit timeout. So +1 to current approach -- 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
Re: [PR] MINOR : Replaced the while loop with TestUtils.waitForCondition [kafka]
chiacyu commented on PR #15678: URL: https://github.com/apache/kafka/pull/15678#issuecomment-2047169130 If the condition doesn't meet within the maxWaitMs, the waitForCondition would throw the assertion failure. Do we need another timeout to handle that? -- 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
Re: [PR] MINOR : Replaced the while loop with TestUtils.waitForCondition [kafka]
chia7712 commented on PR #15678: URL: https://github.com/apache/kafka/pull/15678#issuecomment-2045642331 It seems what we want to fix is the infinite waiting, and so maybe we can just add `Timeout` to this class to fix that? -- 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
Re: [PR] MINOR : Replaced the while loop with TestUtils.waitForCondition [kafka]
chia7712 commented on code in PR #15678:
URL: https://github.com/apache/kafka/pull/15678#discussion_r1557970841
##
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);
Review Comment:
oh, it is `MockTimer` so we call `sleep` to advance the time. please ignore
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-unsubscr...@kafka.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] MINOR : Replaced the while loop with TestUtils.waitForCondition [kafka]
chiacyu commented on code in PR #15678:
URL: https://github.com/apache/kafka/pull/15678#discussion_r1557842174
##
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);
Review Comment:
I tried to remove the sleep but the test would failed because the
readyToAssert.await() would not work as expected. Any ideas on that? 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-unsubscr...@kafka.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] MINOR : Replaced the while loop with TestUtils.waitForCondition [kafka]
chia7712 commented on PR #15678: URL: https://github.com/apache/kafka/pull/15678#issuecomment-2045082221 @chiacyu Could you take a look at my comments? 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-unsubscr...@kafka.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] MINOR : Replaced the while loop with TestUtils.waitForCondition [kafka]
chia7712 commented on code in PR #15678:
URL: https://github.com/apache/kafka/pull/15678#discussion_r1556390789
##
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);
Review Comment:
IIRC, `TestUtils.waitForConditio` includes the `sleep`, so you don't need to
call sleep manually
--
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
[PR] MINOR : Replaced the while loop with TestUtils.waitForCondition [kafka]
chiacyu opened a new pull request, #15678: URL: https://github.com/apache/kafka/pull/15678 We should replace the while loop in some test cases with waitForCondition to prevent infinite looping conditions. -- 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
