You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pulsar.apache.org by GitBox <gi...@apache.org> on 2022/09/01 07:00:34 UTC
[GitHub] [pulsar] HQebupt opened a new pull request, #17389: [fx][flaky-test]Fix PersistentSubscriptionMessageDispatchStreamingDispatcherThrottlingTest.testMultiLevelDispatch
HQebupt opened a new pull request, #17389:
URL: https://github.com/apache/pulsar/pull/17389
### Motivation
Fixed #17388
### Modifications
- Asynchronously produce messages, but it used to send message synchronously in line:468-471
- The 2500ms time limit at least is unreasonable due to the time to consume messages depends on the environment, which cause the failure.
It is better to remove the check, and keep the time limit check 8000ms at most in line:476-478.
https://github.com/apache/pulsar/blob/0517423b0a8d9c981cc5550abfec9e60b55bf3e7/pulsar-broker/src/test/java/org/apache/pulsar/client/api/SubscriptionMessageDispatchThrottlingTest.java#L467-L479
### Verifying this change
- [x] Make sure that the change passes the CI checks.
This change is a trivial rework / code cleanup without any test coverage.
### Does this pull request potentially affect one of the following parts:
If `yes` was chosen, please highlight the changes
- Dependencies (does it add or upgrade a dependency): (no)
- The public API: (no)
- The schema: (no)
- The default values of configurations: (no)
- The wire protocol: (no)
- The rest endpoints: (no)
- The admin cli options: (no)
- Anything that affects deployment: (no)
### Documentation
Check the box below and label this PR (if you have committer privilege).
Need to update docs?
- [x] `doc-not-needed`
--
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: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] MarvinCai commented on a diff in pull request #17389: [fix][flaky-test]Fix PersistentSubscriptionMessageDispatchStreamingDispatcherThrottlingTest.testMultiLevelDispatch
Posted by GitBox <gi...@apache.org>.
MarvinCai commented on code in PR #17389:
URL: https://github.com/apache/pulsar/pull/17389#discussion_r967603515
##########
pulsar-broker/src/test/java/org/apache/pulsar/client/api/SubscriptionMessageDispatchThrottlingTest.java:
##########
@@ -467,15 +467,14 @@ private void testDispatchRate(SubscriptionType subscription,
long start = System.currentTimeMillis();
// Asynchronously produce messages
for (int i = 0; i < numProducedMessages; i++) {
- producer.send(new byte[expectRate / 10]);
+ producer.sendAsync(new byte[expectRate / 10]);
}
latch.await();
Assert.assertEquals(totalReceived.get(), numProducedMessages, 10);
long end = System.currentTimeMillis();
log.info("-- end - start: {} ", end - start);
// first 10 messages, which equals receiverQueueSize, will not wait.
- Assert.assertTrue((end - start) >= 2500);
Review Comment:
can you help explain this new assert? before each message size is rate/10 and we send 30 message, so should take at least 3 second to receive all, but I don't quite understand what this new assert is trying to prove?
--
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: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] congbobo184 commented on a diff in pull request #17389: [fix][flaky-test]Fix PersistentSubscriptionMessageDispatchStreamingDispatcherThrottlingTest.testMultiLevelDispatch
Posted by GitBox <gi...@apache.org>.
congbobo184 commented on code in PR #17389:
URL: https://github.com/apache/pulsar/pull/17389#discussion_r980786813
##########
pulsar-broker/src/test/java/org/apache/pulsar/client/api/SubscriptionMessageDispatchThrottlingTest.java:
##########
@@ -467,16 +467,16 @@ private void testDispatchRate(SubscriptionType subscription,
long start = System.currentTimeMillis();
// Asynchronously produce messages
for (int i = 0; i < numProducedMessages; i++) {
- producer.send(new byte[expectRate / 10]);
+ producer.sendAsync(new byte[expectRate / 10]);
}
latch.await();
Assert.assertEquals(totalReceived.get(), numProducedMessages, 10);
long end = System.currentTimeMillis();
log.info("-- end - start: {} ", end - start);
// first 10 messages, which equals receiverQueueSize, will not wait.
- Assert.assertTrue((end - start) >= 2500);
- Assert.assertTrue((end - start) <= 8000);
+ Assert.assertEquals((end - start), numProducedMessages * expectRate / 10,
Review Comment:
Why is this assertion so certain, it may cause the test to become more unstable
--
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: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] mattisonchao commented on a diff in pull request #17389: [fix][flaky-test]Fix PersistentSubscriptionMessageDispatchStreamingDispatcherThrottlingTest.testMultiLevelDispatch
Posted by GitBox <gi...@apache.org>.
mattisonchao commented on code in PR #17389:
URL: https://github.com/apache/pulsar/pull/17389#discussion_r960319429
##########
pulsar-broker/src/test/java/org/apache/pulsar/client/api/SubscriptionMessageDispatchThrottlingTest.java:
##########
@@ -467,15 +467,14 @@ private void testDispatchRate(SubscriptionType subscription,
long start = System.currentTimeMillis();
// Asynchronously produce messages
for (int i = 0; i < numProducedMessages; i++) {
- producer.send(new byte[expectRate / 10]);
+ producer.sendAsync(new byte[expectRate / 10]);
}
latch.await();
Assert.assertEquals(totalReceived.get(), numProducedMessages, 10);
long end = System.currentTimeMillis();
log.info("-- end - start: {} ", end - start);
// first 10 messages, which equals receiverQueueSize, will not wait.
- Assert.assertTrue((end - start) >= 2500);
Review Comment:
I think we can't remove this line because we have to use it to verify the rate limiter.
Could you get the actual number here? Maybe 2000 is better or not?
--
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: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] HQebupt commented on a diff in pull request #17389: [fix][flaky-test]Fix PersistentSubscriptionMessageDispatchStreamingDispatcherThrottlingTest.testMultiLevelDispatch
Posted by GitBox <gi...@apache.org>.
HQebupt commented on code in PR #17389:
URL: https://github.com/apache/pulsar/pull/17389#discussion_r984187071
##########
pulsar-broker/src/test/java/org/apache/pulsar/client/api/SubscriptionMessageDispatchThrottlingTest.java:
##########
@@ -467,16 +467,16 @@ private void testDispatchRate(SubscriptionType subscription,
long start = System.currentTimeMillis();
// Asynchronously produce messages
for (int i = 0; i < numProducedMessages; i++) {
- producer.send(new byte[expectRate / 10]);
+ producer.sendAsync(new byte[expectRate / 10]);
}
latch.await();
Assert.assertEquals(totalReceived.get(), numProducedMessages, 10);
long end = System.currentTimeMillis();
log.info("-- end - start: {} ", end - start);
// first 10 messages, which equals receiverQueueSize, will not wait.
- Assert.assertTrue((end - start) >= 2500);
- Assert.assertTrue((end - start) <= 8000);
+ Assert.assertEquals((end - start), numProducedMessages * expectRate / 10,
Review Comment:
It make sense.
--
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: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] HQebupt commented on a diff in pull request #17389: [fix][flaky-test]Fix PersistentSubscriptionMessageDispatchStreamingDispatcherThrottlingTest.testMultiLevelDispatch
Posted by GitBox <gi...@apache.org>.
HQebupt commented on code in PR #17389:
URL: https://github.com/apache/pulsar/pull/17389#discussion_r984187456
##########
pulsar-broker/src/test/java/org/apache/pulsar/client/api/SubscriptionMessageDispatchThrottlingTest.java:
##########
@@ -467,15 +467,14 @@ private void testDispatchRate(SubscriptionType subscription,
long start = System.currentTimeMillis();
// Asynchronously produce messages
for (int i = 0; i < numProducedMessages; i++) {
- producer.send(new byte[expectRate / 10]);
+ producer.sendAsync(new byte[expectRate / 10]);
}
latch.await();
Assert.assertEquals(totalReceived.get(), numProducedMessages, 10);
long end = System.currentTimeMillis();
log.info("-- end - start: {} ", end - start);
// first 10 messages, which equals receiverQueueSize, will not wait.
- Assert.assertTrue((end - start) >= 2500);
Review Comment:
The test case is correct. And I found the failure is due to dataProvider. I update the PR. PTAL.
--
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: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] HQebupt commented on a diff in pull request #17389: [fix][flaky-test]Fix PersistentSubscriptionMessageDispatchStreamingDispatcherThrottlingTest.testMultiLevelDispatch
Posted by GitBox <gi...@apache.org>.
HQebupt commented on code in PR #17389:
URL: https://github.com/apache/pulsar/pull/17389#discussion_r960771333
##########
pulsar-broker/src/test/java/org/apache/pulsar/client/api/SubscriptionMessageDispatchThrottlingTest.java:
##########
@@ -467,15 +467,14 @@ private void testDispatchRate(SubscriptionType subscription,
long start = System.currentTimeMillis();
// Asynchronously produce messages
for (int i = 0; i < numProducedMessages; i++) {
- producer.send(new byte[expectRate / 10]);
+ producer.sendAsync(new byte[expectRate / 10]);
}
latch.await();
Assert.assertEquals(totalReceived.get(), numProducedMessages, 10);
long end = System.currentTimeMillis();
log.info("-- end - start: {} ", end - start);
// first 10 messages, which equals receiverQueueSize, will not wait.
- Assert.assertTrue((end - start) >= 2500);
Review Comment:
I agree with you. The longer the time, the more accurate the rate.
--
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: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] HQebupt commented on pull request #17389: [fix][flaky-test]Fix PersistentSubscriptionMessageDispatchStreamingDispatcherThrottlingTest.testMultiLevelDispatch
Posted by GitBox <gi...@apache.org>.
HQebupt commented on PR #17389:
URL: https://github.com/apache/pulsar/pull/17389#issuecomment-1263078047
@MarvinCai @congbobo184 @mattisonchao I found the failure it is due to dataProvider. I split the subscriptions into two separate data provider. It works. PTAL
--
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: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] HQebupt closed pull request #17389: [fix][flaky-test]Fix PersistentSubscriptionMessageDispatchStreamingDispatcherThrottlingTest.testMultiLevelDispatch
Posted by GitBox <gi...@apache.org>.
HQebupt closed pull request #17389: [fix][flaky-test]Fix PersistentSubscriptionMessageDispatchStreamingDispatcherThrottlingTest.testMultiLevelDispatch
URL: https://github.com/apache/pulsar/pull/17389
--
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: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org