[GitHub] [activemq-artemis] michaelpearce-gain commented on a change in pull request #3029: ARTEMIS-2664 Fix the credits acquiring.

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

michaelpearce-gain commented on a change in pull request #3029: ARTEMIS-2664 Fix the credits acquiring.
URL: https://github.com/apache/activemq-artemis/pull/3029#discussion_r395222254
 
 

 ##########
 File path: artemis-protocols/artemis-openwire-protocol/src/main/java/org/apache/activemq/artemis/core/protocol/openwire/amq/AMQConsumer.java
 ##########
 @@ -306,15 +306,12 @@ public void acknowledge(MessageAck ack) throws Exception {
       List<MessageReference> ackList = serverConsumer.getDeliveringReferencesBasedOnProtocol(removeReferences, first, last);
 
       if (removeReferences && (ack.isIndividualAck() || ack.isStandardAck() || ack.isPoisonAck())) {
-         this.deliveredAcks.getAndUpdate(deliveredAcks -> {
-            if (deliveredAcks >= ackList.size()) {
-               return deliveredAcks - ackList.size();
-            }
-
-            acquireCredit(ackList.size() - deliveredAcks);
+         int previousDeliveredAcks = this.deliveredAcks.getAndUpdate(
+            deliveredAcks -> deliveredAcks > ackList.size() ? deliveredAcks - ackList.size() : 0);
 
 Review comment:
   My concern is not concurrent updates, but that updates can occur on different thread (possibly even different core) and thus the update isnt visible. 
   
   Reading the comment it seems there has been made an assumption based on EpollEventLoop. What about KQueue or even if on windows, where neither is present? Are the guarantees the events come on same thread.

----------------------------------------------------------------
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.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services