mattisonchao commented on code in PR #17483:
URL: https://github.com/apache/pulsar/pull/17483#discussion_r966675008
##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/service/persistent/PersistentDispatcherMultipleConsumers.java:
##########
@@ -634,7 +634,9 @@ protected synchronized boolean
trySendMessagesToConsumers(ReadType readType, Lis
// round-robin dispatch batch size for this consumer
int availablePermits = c.isWritable() ? c.getAvailablePermits() :
1;
if (c.getMaxUnackedMessages() > 0) {
- availablePermits = Math.min(availablePermits,
c.getMaxUnackedMessages() - c.getUnackedMessages());
+ // Avoid negative number
+ int remainUnAckedMessages = Math.max(c.getMaxUnackedMessages()
- c.getUnackedMessages(), 0);
Review Comment:
>Do we understand why c.getUnackedMessages() is greater than
c.getMaxUnackedMessages()?
Because in the current implementation, we will add `unackedMessages` after
sent messages. if we use batch messages, the `unackedMessages` is probably
greater than `MaxUnackedMessages`.
Plus, #16670 #16718 want to add this check to limit the consumer.
>Based on the names, that seems like there is another bug if
c.getMaxUnackedMessages() - c.getUnackedMessages() is negative.
Sure, But I find we have two sections using it. And I fix It all in this PR.
>Perhaps the idea that the maxUnackedMessages is just a soft limit that we
shouldn't exceed by too much?
Yes, maybe we have another method to help more robust support it, but it
looks like it is another improvement.
--
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]
