lhotari commented on code in PR #25445:
URL: https://github.com/apache/pulsar/pull/25445#discussion_r3018273783
##########
pulsar-broker/src/test/java/org/apache/pulsar/client/api/KeySharedSubscriptionTest.java:
##########
@@ -2290,20 +2281,14 @@ public void
testReadAheadLimit(KeySharedImplementationType impl) throws Exceptio
remainingMessageValues.add(i);
}
- checkLimit.run();
-
- Thread.sleep(pauseTime);
- checkLimit.run();
-
- Thread.sleep(pauseTime);
- checkLimit.run();
+ // Wait for messages to be dispatched and verify limit is respected
+ Awaitility.await().untilAsserted(checkLimit::run);
Review Comment:
Using Awaitility here changes the original logic. The intention is to run
the check several times and verify that the limit isn't exceeded. pauseTime is
only 100ms so there shouldn't be a need to change the original logic.
##########
pulsar-broker/src/test/java/org/apache/pulsar/client/api/KeySharedSubscriptionTest.java:
##########
@@ -2290,20 +2281,14 @@ public void
testReadAheadLimit(KeySharedImplementationType impl) throws Exceptio
remainingMessageValues.add(i);
}
- checkLimit.run();
-
- Thread.sleep(pauseTime);
- checkLimit.run();
-
- Thread.sleep(pauseTime);
- checkLimit.run();
+ // Wait for messages to be dispatched and verify limit is respected
+ Awaitility.await().untilAsserted(checkLimit::run);
// resume c1 and c3
c1.resume();
c3.resume();
- Thread.sleep(pauseTime);
- checkLimit.run();
+ Awaitility.await().untilAsserted(checkLimit::run);
Review Comment:
Using Awaitility here changes the original logic. The intention is to run
the check several times and verify that the limit isn't exceeded. pauseTime is
only 100ms so there shouldn't be a need to change the original logic.
##########
pulsar-broker/src/test/java/org/apache/pulsar/client/api/KeySharedSubscriptionTest.java:
##########
@@ -1218,11 +1211,7 @@ public void
testOrderingWithConsumerListener(KeySharedImplementationType impl, b
.subscriptionName(subName)
.subscriptionType(SubscriptionType.Key_Shared)
.messageListener((MessageListener<Integer>) (consumer1, msg)
-> {
- try {
- Thread.sleep(random.nextInt(5));
Review Comment:
IIRC, In this particular case, the random delay triggered the problem.
removing this will change the original intention. It's my bad that this hasn't
been properly documented initially in test code. I'd assume that the PR has
some description.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
