[GitHub] [activemq-artemis] gemmellr commented on a change in pull request #3635: ARTEMIS-2007 - allow redistribution if there are unmatched messages p…

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

gemmellr commented on a change in pull request #3635:
URL: https://github.com/apache/activemq-artemis/pull/3635#discussion_r681731532



##########
File path: artemis-server/src/main/java/org/apache/activemq/artemis/core/server/impl/QueueImpl.java
##########
@@ -1567,7 +1566,14 @@ protected void finalize() throws Throwable {
 
    @Override
    public int getConsumerCount() {
-      return consumers.size();
+      // we don't want to count the redistributor, it is an internal transient entry in the consumer list
+      // check for presence before checking consumers to align with use of set()
+      final boolean compensateForPresenceOfRedistributor = redistributorUpdater.get(this) != null;
+      int size = consumers.size();
+      if (size > 0 && compensateForPresenceOfRedistributor) {
+         size--;
+      }
+      return size;

Review comment:
       Sine this isnt done under lock and both the redistributor and consumers list could be changing independently, it seems quite racey? Seems to be used in some important places so I'd wonder if that is a problem.
   
   Given that 'consumers' is actually a custom structure, and updates to its underlying structure are done under lock, it coudl perhaps be made to handle this internally (and e.g add a specific compensating size method)?

##########
File path: artemis-server/src/main/java/org/apache/activemq/artemis/core/server/impl/QueueImpl.java
##########
@@ -140,6 +139,7 @@
    private static final AtomicLongFieldUpdater<QueueImpl> dispatchStartTimeUpdater = AtomicLongFieldUpdater.newUpdater(QueueImpl.class, "dispatchStartTime");
    private static final AtomicLongFieldUpdater<QueueImpl> consumerRemovedTimestampUpdater = AtomicLongFieldUpdater.newUpdater(QueueImpl.class, "consumerRemovedTimestamp");
    private static final AtomicReferenceFieldUpdater<QueueImpl, Filter> filterUpdater = AtomicReferenceFieldUpdater.newUpdater(QueueImpl.class, Filter.class, "filter");
+   private static final AtomicReferenceFieldUpdater<QueueImpl, ConsumerHolder> redistributorUpdater = AtomicReferenceFieldUpdater.newUpdater(QueueImpl.class, ConsumerHolder.class, "redistributor");

Review comment:
       Is this needed? Seems like there are no CAS etc operations done on it, just straight sets, and all actual uses of the redistributor field except one are done under synchronized methods/blocks, except the use in getConsumerCount where it just does a get with this updater vs using the volatile-anyway field. (The subsequent usage there is perhaps not so safe as it isnt under lock)




-- 
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: gitbox-unsubscribe@activemq.apache.org

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