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 2021/12/15 09:11:04 UTC
[GitHub] [pulsar] Demogorgon314 opened a new pull request #13332: [pulsar-client] Fix multi topic reader has message available behavior
Demogorgon314 opened a new pull request #13332:
URL: https://github.com/apache/pulsar/pull/13332
### Motivation
When we use a multi-topic reader, the `hasMessageAvailable` method might have the wrong behavior, since the multi-topics consumer receives all messages from the single-topic consumer, the single-topic consumer `hasMessageAvailable` might always be `false` (The lastDequeuedMessageId reach to the end of the queue, all message enqueue to multi-topic consumer's `incomingMessages` queue).
We should check the multi-topics consumer `incomingMessages` size > 0 when calling `hasMessageAvailable `.
### Modifications
1. Add a check of `incomingMessages` size > 0
2. Add units test `testHasMessageAvailableAsync` to verify the behavior.
### Documentation
Need to update docs?
- [ ] `doc-required`
(If you need help on updating docs, create a doc issue)
- [x] `no-need-doc`
(Please explain why)
- [ ] `doc`
(If this PR contains doc 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: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] Demogorgon314 commented on a change in pull request #13332: [pulsar-client] Fix multi topic reader has message available behavior
Posted by GitBox <gi...@apache.org>.
Demogorgon314 commented on a change in pull request #13332:
URL: https://github.com/apache/pulsar/pull/13332#discussion_r771177881
##########
File path: pulsar-client/src/main/java/org/apache/pulsar/client/impl/MultiTopicsConsumerImpl.java
##########
@@ -779,7 +779,7 @@ public boolean hasMessageAvailable() throws PulsarClientException {
if (exception != null) {
completableFuture.completeExceptionally(exception);
} else {
- completableFuture.complete(hasMessageAvailable.get());
+ completableFuture.complete(hasMessageAvailable.get() || numMessagesInQueue() > 0);
Review comment:
You're right. I think moving to the top of the method doesn't change the behavior.
In the previous implementation, even if `hasMessageAvailableAsync` is true, the `readNext()` still might blocking.
--
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] lhotari commented on pull request #13332: [pulsar-client] Fix multi topic reader has message available behavior
Posted by GitBox <gi...@apache.org>.
lhotari commented on pull request #13332:
URL: https://github.com/apache/pulsar/pull/13332#issuecomment-1004854375
This PR introduced a new flaky test, #13605 . Please fix it. It might be a matter of increasing the timeout from 10000 ms to a higher value.
--
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] Demogorgon314 commented on pull request #13332: [pulsar-client] Fix multi topic reader has message available behavior
Posted by GitBox <gi...@apache.org>.
Demogorgon314 commented on pull request #13332:
URL: https://github.com/apache/pulsar/pull/13332#issuecomment-994833949
/pulsarbot run-failure-checks
--
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] Demogorgon314 commented on pull request #13332: [pulsar-client] Fix multi topic reader has message available behavior
Posted by GitBox <gi...@apache.org>.
Demogorgon314 commented on pull request #13332:
URL: https://github.com/apache/pulsar/pull/13332#issuecomment-997133221
/pulsarbot run-failure-checks
--
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] eolivelli commented on a change in pull request #13332: [pulsar-client] Fix multi topic reader has message available behavior
Posted by GitBox <gi...@apache.org>.
eolivelli commented on a change in pull request #13332:
URL: https://github.com/apache/pulsar/pull/13332#discussion_r771131501
##########
File path: pulsar-client/src/main/java/org/apache/pulsar/client/impl/MultiTopicsConsumerImpl.java
##########
@@ -779,7 +779,7 @@ public boolean hasMessageAvailable() throws PulsarClientException {
if (exception != null) {
completableFuture.completeExceptionally(exception);
} else {
- completableFuture.complete(hasMessageAvailable.get());
+ completableFuture.complete(hasMessageAvailable.get() || numMessagesInQueue() > 0);
Review comment:
What about moving the check on the number of messages on the top of the method?
If we have messages we can skip doing the other things.
Bu the way I am not sure if doing so would alter the way it works significantly, as in case of having messages we won't call hasMessageAvailableAsync on the wrapped consumers
Did you think about this possibility?
--
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] Demogorgon314 commented on pull request #13332: [pulsar-client] Fix multi topic reader has message available behavior
Posted by GitBox <gi...@apache.org>.
Demogorgon314 commented on pull request #13332:
URL: https://github.com/apache/pulsar/pull/13332#issuecomment-996612741
/pulsarbot run-failure-checks
--
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] codelipenghui merged pull request #13332: [pulsar-client] Fix multi topic reader has message available behavior
Posted by GitBox <gi...@apache.org>.
codelipenghui merged pull request #13332:
URL: https://github.com/apache/pulsar/pull/13332
--
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