michaeljmarshall commented on code in PR #17237:
URL: https://github.com/apache/pulsar/pull/17237#discussion_r953131445
##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/service/persistent/PersistentDispatcherSingleActiveConsumer.java:
##########
@@ -332,12 +332,17 @@ public void redeliverUnacknowledgedMessages(Consumer
consumer, List<PositionImpl
}
@Override
- protected void readMoreEntries(Consumer consumer) {
+ protected synchronized void readMoreEntries(Consumer consumer) {
// consumer can be null when all consumers are disconnected from
broker.
// so skip reading more entries if currently there is no active
consumer.
if (null == consumer) {
return;
}
+ if (havePendingRead) {
Review Comment:
I see several parts of the code that call this `readMoreEntries` method
after verifying that `havePendingRead` is `true`. I haven't yet verified the
details of each call, but it seems like we should clean up those guards if we
are moving the logic into this method.
##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/service/persistent/PersistentDispatcherSingleActiveConsumer.java:
##########
@@ -332,12 +332,17 @@ public void redeliverUnacknowledgedMessages(Consumer
consumer, List<PositionImpl
}
@Override
- protected void readMoreEntries(Consumer consumer) {
+ protected synchronized void readMoreEntries(Consumer consumer) {
Review Comment:
It seems like we might not want to acquire the lock before the `consumer`
null check.
--
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]
