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



##########
File path: artemis-server/src/main/java/org/apache/activemq/artemis/core/server/impl/QueueImpl.java
##########
@@ -3055,6 +3058,10 @@ private boolean deliver() {
                } else if (status == HandleStatus.NO_MATCH) {
                   // nothing to be done on this case, the iterators will just jump next
                   consumers.reset();
+                  numNoMatch++;
+                  if (numNoMatch == consumerCount) {

Review comment:
       The JIRA and code seem like it would be trying for 'no consumers matched message', but since the consumers iterator is reset just before this, aren't many/most of the attempts leading to this count incrementing going to be performed against the same consumer(s, but mostly first one), rather than checking all the consumers?

##########
File path: artemis-server/src/main/java/org/apache/activemq/artemis/core/server/impl/QueueImpl.java
##########
@@ -3066,7 +3073,7 @@ private boolean deliver() {
             } else if (!consumers.hasNext()) {
                // Round robin'd all
 
-               if (noDelivery == this.consumers.size()) {
+               if (noDelivery == consumerCount) {

Review comment:
       This seems like a potentially significant change in behaviour. I wonder about its safety a bit.
   
   The previous version would see any updates to the consumer list size that happened while this method was running, whereas the new version takes a snapshot of the count before starting the loop and retains it for the rest of the time. It retrieves the value outside the synchronized block which the loop goes in and out of as it operates. So it feels like it could be wrong to begin with, and then not reflect changes which the iterator itself can get (from e.g. the various resets it does), and which it does so while under synchronization.




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