[GitHub] [pulsar] mattisonchao commented on a diff in pull request #19167: [improve][broker] Add ref count for sticky hash to optimize the performance of Key_Shared subscription

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

mattisonchao commented on code in PR #19167:
URL: https://github.com/apache/pulsar/pull/19167#discussion_r1065381414


##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/service/persistent/MessageRedeliveryController.java:
##########
@@ -23,52 +23,86 @@
 import java.util.List;
 import java.util.NavigableSet;
 import java.util.Set;
-import java.util.concurrent.atomic.AtomicBoolean;
 import org.apache.bookkeeper.mledger.impl.PositionImpl;
+import org.apache.bookkeeper.util.collections.ConcurrentLongLongHashMap;
 import org.apache.pulsar.common.util.collections.ConcurrentLongLongPairHashMap;
 import org.apache.pulsar.common.util.collections.ConcurrentLongLongPairHashMap.LongPair;
 import org.apache.pulsar.utils.ConcurrentBitmapSortedLongPairSet;
 
+/**
+ * The MessageRedeliveryController is a non-thread-safe container for maintaining the redelivery messages.
+ */
 public class MessageRedeliveryController {

Review Comment:
   `@NotThreadSafe` annotation will be more clear.



##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/service/persistent/MessageRedeliveryController.java:
##########
@@ -23,52 +23,86 @@
 import java.util.List;
 import java.util.NavigableSet;
 import java.util.Set;
-import java.util.concurrent.atomic.AtomicBoolean;
 import org.apache.bookkeeper.mledger.impl.PositionImpl;
+import org.apache.bookkeeper.util.collections.ConcurrentLongLongHashMap;
 import org.apache.pulsar.common.util.collections.ConcurrentLongLongPairHashMap;
 import org.apache.pulsar.common.util.collections.ConcurrentLongLongPairHashMap.LongPair;
 import org.apache.pulsar.utils.ConcurrentBitmapSortedLongPairSet;
 
+/**
+ * The MessageRedeliveryController is a non-thread-safe container for maintaining the redelivery messages.
+ */
 public class MessageRedeliveryController {
+
+    private final boolean allowOutOfOrderDelivery;
     private final ConcurrentBitmapSortedLongPairSet messagesToRedeliver;
     private final ConcurrentLongLongPairHashMap hashesToBeBlocked;
+    private final ConcurrentLongLongHashMap hashesRefCount;
 
     public MessageRedeliveryController(boolean allowOutOfOrderDelivery) {
+        this.allowOutOfOrderDelivery = allowOutOfOrderDelivery;
         this.messagesToRedeliver = new ConcurrentBitmapSortedLongPairSet();
-        this.hashesToBeBlocked = allowOutOfOrderDelivery
-                ? null
-                : ConcurrentLongLongPairHashMap
+        if (!allowOutOfOrderDelivery) {
+            this.hashesToBeBlocked = ConcurrentLongLongPairHashMap
                     .newBuilder().concurrencyLevel(2).expectedItems(128).autoShrink(true).build();
+            this.hashesRefCount = ConcurrentLongLongHashMap
+                    .newBuilder().concurrencyLevel(2).expectedItems(128).autoShrink(true).build();
+        } else {
+            this.hashesToBeBlocked = null;

Review Comment:
   I'm not sure if this `null` assignment is good, I just wondering if is it will introduce potential NPE in the future.



-- 
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