[GitHub] [pulsar] michaeljmarshall commented on a diff in pull request #17237: [fix][broker] Fix dispatch the duplicated message with `Exclusive` mode.

[GitBox <gi...@apache.org>]

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: commits-unsubscribe@pulsar.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org