You are viewing a plain text version of this content. The canonical link for it is here.
Posted to gitbox@activemq.apache.org by GitBox <gi...@apache.org> on 2020/03/18 11:52:22 UTC
[GitHub] [activemq-artemis] brusdev opened a new pull request #3029:
ARTEMIS-2664 Fix the delivered acks decrement.
brusdev opened a new pull request #3029: ARTEMIS-2664 Fix the delivered acks decrement.
URL: https://github.com/apache/activemq-artemis/pull/3029
----------------------------------------------------------------
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
[GitHub] [activemq-artemis] clebertsuconic commented on a change in pull
request #3029: ARTEMIS-2664 Fix the delivered acks decrement.
Posted by GitBox <gi...@apache.org>.
clebertsuconic commented on a change in pull request #3029: ARTEMIS-2664 Fix the delivered acks decrement.
URL: https://github.com/apache/activemq-artemis/pull/3029#discussion_r394513209
##########
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:
I guess the commit message is not clear enough.. I think the problem is the nubmer of times acquiredCredits is called?
I would be a bit more descriptive on the commit message if this is the issue.
----------------------------------------------------------------
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
[GitHub] [activemq-artemis] michaelandrepearce commented on a change in pull
request #3029: ARTEMIS-2664 Fix the delivered acks decrement.
Posted by GitBox <gi...@apache.org>.
michaelandrepearce commented on a change in pull request #3029: ARTEMIS-2664 Fix the delivered acks decrement.
URL: https://github.com/apache/activemq-artemis/pull/3029#discussion_r394504659
##########
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:
why not >= as was before?
----------------------------------------------------------------
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
[GitHub] [activemq-artemis] brusdev commented on a change in pull request
#3029: ARTEMIS-2664 Fix the delivered acks decrement.
Posted by GitBox <gi...@apache.org>.
brusdev commented on a change in pull request #3029: ARTEMIS-2664 Fix the delivered acks decrement.
URL: https://github.com/apache/activemq-artemis/pull/3029#discussion_r394952121
##########
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:
I created https://github.com/brusdev/activemq-artemis/tree/fix_delivered_acks_decrement_simplified to check the simplified version and I executed all openwire tests in integration-tests against it and LGTM.
----------------------------------------------------------------
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
[GitHub] [activemq-artemis] michaelpearce-gain commented on issue #3029:
ARTEMIS-2664 Fix the credits acquiring.
Posted by GitBox <gi...@apache.org>.
michaelpearce-gain commented on issue #3029: ARTEMIS-2664 Fix the credits acquiring.
URL: https://github.com/apache/activemq-artemis/pull/3029#issuecomment-605369686
@franz1981 are you merging then?
----------------------------------------------------------------
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
[GitHub] [activemq-artemis] michaelandrepearce commented on a change in pull
request #3029: ARTEMIS-2664 Fix the credits acquiring.
Posted by GitBox <gi...@apache.org>.
michaelandrepearce commented on a change in pull request #3029: ARTEMIS-2664 Fix the credits acquiring.
URL: https://github.com/apache/activemq-artemis/pull/3029#discussion_r395230231
##########
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:
As long as were 100%. Im just worried after past issues
----------------------------------------------------------------
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
[GitHub] [activemq-artemis] franz1981 commented on a change in pull request
#3029: ARTEMIS-2664 Fix the credits acquiring.
Posted by GitBox <gi...@apache.org>.
franz1981 commented on a change in pull request #3029: ARTEMIS-2664 Fix the credits acquiring.
URL: https://github.com/apache/activemq-artemis/pull/3029#discussion_r395194815
##########
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:
The original code wasn't having any AtomicInteger declaration at all
----------------------------------------------------------------
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
[GitHub] [activemq-artemis] franz1981 commented on a change in pull request
#3029: ARTEMIS-2664 Fix the credits acquiring.
Posted by GitBox <gi...@apache.org>.
franz1981 commented on a change in pull request #3029: ARTEMIS-2664 Fix the credits acquiring.
URL: https://github.com/apache/activemq-artemis/pull/3029#discussion_r395208591
##########
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:
We can add some check too in form of assert to get the CI to fail I'd the assumption won't hold 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [activemq-artemis] franz1981 commented on a change in pull request
#3029: ARTEMIS-2664 Fix the credits acquiring.
Posted by GitBox <gi...@apache.org>.
franz1981 commented on a change in pull request #3029: ARTEMIS-2664 Fix the credits acquiring.
URL: https://github.com/apache/activemq-artemis/pull/3029#discussion_r395268455
##########
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:
That's perfectly reasonable indeed, from any pov I agree :)
In general I often see ownership and threading model not always clear in our code base so I would like to simplify things when we have the chance to do it to make easier the life of future contributors (including ourselves in the future). Probably thinking on some check/assert is a nice addition to make this more robust over time...
----------------------------------------------------------------
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
[GitHub] [activemq-artemis] brusdev commented on a change in pull request
#3029: ARTEMIS-2664 Fix the delivered acks decrement.
Posted by GitBox <gi...@apache.org>.
brusdev commented on a change in pull request #3029: ARTEMIS-2664 Fix the delivered acks decrement.
URL: https://github.com/apache/activemq-artemis/pull/3029#discussion_r394895116
##########
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:
@michaelandrepearce maybe I could simplify this replacing the AtomicInteger with an `int`. Indeed `deliveredAcks` is used only by the `acknowledge` method and it is only executed by the `EpollEventLoop` thread bounded with the relative connection channel. WDYT?
----------------------------------------------------------------
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
[GitHub] [activemq-artemis] franz1981 commented on a change in pull request
#3029: ARTEMIS-2664 Fix the credits acquiring.
Posted by GitBox <gi...@apache.org>.
franz1981 commented on a change in pull request #3029: ARTEMIS-2664 Fix the credits acquiring.
URL: https://github.com/apache/activemq-artemis/pull/3029#discussion_r395204390
##########
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:
I am not sure is a good idea to developing it assuming a concurrent access that is proved to not be present: it means making the core more complex and the testing should account and stress that too.
@brusdev has already verified that the code can be single-threaded so I vote for the simplest solution here.
----------------------------------------------------------------
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
[GitHub] [activemq-artemis] brusdev commented on a change in pull request
#3029: ARTEMIS-2664 Fix the delivered acks decrement.
Posted by GitBox <gi...@apache.org>.
brusdev commented on a change in pull request #3029: ARTEMIS-2664 Fix the delivered acks decrement.
URL: https://github.com/apache/activemq-artemis/pull/3029#discussion_r394883532
##########
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:
@michaelandrepearce before was `>=` to not call acquireCredit with 0.
----------------------------------------------------------------
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
[GitHub] [activemq-artemis] franz1981 commented on a change in pull request
#3029: ARTEMIS-2664 Fix the credits acquiring.
Posted by GitBox <gi...@apache.org>.
franz1981 commented on a change in pull request #3029: ARTEMIS-2664 Fix the credits acquiring.
URL: https://github.com/apache/activemq-artemis/pull/3029#discussion_r395224396
##########
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:
The event loop executions re the same Netty channel always happen on the same Thread (is a strong assumption in Netty or FastThreadLocal wouldn't work) so as-single-threaded, visibility shouldn't be a concern due to it.
----------------------------------------------------------------
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
[GitHub] [activemq-artemis] franz1981 commented on a change in pull request
#3029: ARTEMIS-2664 Fix the credits acquiring.
Posted by GitBox <gi...@apache.org>.
franz1981 commented on a change in pull request #3029: ARTEMIS-2664 Fix the credits acquiring.
URL: https://github.com/apache/activemq-artemis/pull/3029#discussion_r395204390
##########
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:
I am not sure is a good idea to developing it assuming a concurrent access that is proved to not be present: it means making the code more complex for no reasons and the testing should account and stress that too.
@brusdev has already verified that the code can be single-threaded so I vote for the simplest solution here.
----------------------------------------------------------------
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
[GitHub] [activemq-artemis] michaelandrepearce commented on a change in pull
request #3029: ARTEMIS-2664 Fix the credits acquiring.
Posted by GitBox <gi...@apache.org>.
michaelandrepearce commented on a change in pull request #3029: ARTEMIS-2664 Fix the credits acquiring.
URL: https://github.com/apache/activemq-artemis/pull/3029#discussion_r395192218
##########
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:
I would go with the least change. More changes increase risk of introducing new or other bugs.
Using int isn't thread safe.
----------------------------------------------------------------
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
[GitHub] [activemq-artemis] michaelandrepearce commented on a change in pull
request #3029: ARTEMIS-2664 Fix the delivered acks decrement.
Posted by GitBox <gi...@apache.org>.
michaelandrepearce commented on a change in pull request #3029: ARTEMIS-2664 Fix the delivered acks decrement.
URL: https://github.com/apache/activemq-artemis/pull/3029#discussion_r394507014
##########
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:
i guess it still result in same result as 0, just be interesting if there was any reason.
----------------------------------------------------------------
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
[GitHub] [activemq-artemis] michaelandrepearce commented on a change in pull
request #3029: ARTEMIS-2664 Fix the delivered acks decrement.
Posted by GitBox <gi...@apache.org>.
michaelandrepearce commented on a change in pull request #3029: ARTEMIS-2664 Fix the delivered acks decrement.
URL: https://github.com/apache/activemq-artemis/pull/3029#discussion_r394425089
##########
File path: artemis-protocols/artemis-openwire-protocol/src/main/java/org/apache/activemq/artemis/core/protocol/openwire/amq/AMQConsumer.java
##########
@@ -306,18 +306,15 @@ 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();
- }
-
+ if (deliveredAcks < ackList.size()) {
acquireCredit(ackList.size() - deliveredAcks);
-
- return 0;
- });
+ deliveredAcks = 0;
+ } else {
+ deliveredAcks -= ackList.size();
+ }
} else {
if (ack.isDeliveredAck()) {
- this.deliveredAcks.addAndGet(ack.getMessageCount());
+ this.deliveredAcks += ack.getMessageCount();
Review comment:
this isn't concurrent safe, if different threads modify. thus why it was atomic
----------------------------------------------------------------
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
[GitHub] [activemq-artemis] michaelandrepearce commented on a change in pull
request #3029: ARTEMIS-2664 Fix the credits acquiring.
Posted by GitBox <gi...@apache.org>.
michaelandrepearce commented on a change in pull request #3029: ARTEMIS-2664 Fix the credits acquiring.
URL: https://github.com/apache/activemq-artemis/pull/3029#discussion_r395196321
##########
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:
Id def be a -1 moving from atomic
----------------------------------------------------------------
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
[GitHub] [activemq-artemis] michaelpearce-gain commented on a change in pull
request #3029: ARTEMIS-2664 Fix the credits acquiring.
Posted by 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
[GitHub] [activemq-artemis] asfgit closed pull request #3029: ARTEMIS-2664
Fix the credits acquiring.
Posted by GitBox <gi...@apache.org>.
asfgit closed pull request #3029: ARTEMIS-2664 Fix the credits acquiring.
URL: https://github.com/apache/activemq-artemis/pull/3029
----------------------------------------------------------------
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
[GitHub] [activemq-artemis] michaelandrepearce commented on a change in pull
request #3029: ARTEMIS-2664 Fix the delivered acks decrement.
Posted by GitBox <gi...@apache.org>.
michaelandrepearce commented on a change in pull request #3029: ARTEMIS-2664 Fix the delivered acks decrement.
URL: https://github.com/apache/activemq-artemis/pull/3029#discussion_r394424651
##########
File path: artemis-protocols/artemis-openwire-protocol/src/main/java/org/apache/activemq/artemis/core/protocol/openwire/amq/AMQConsumer.java
##########
@@ -68,7 +68,7 @@
private int prefetchSize;
private final AtomicInteger currentWindow;
- private final AtomicInteger deliveredAcks;
+ private int deliveredAcks;
Review comment:
please leave this atomic, to avoid concurrency visibility issues with different threads.
----------------------------------------------------------------
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
[GitHub] [activemq-artemis] brusdev commented on a change in pull request
#3029: ARTEMIS-2664 Fix the credits acquiring.
Posted by GitBox <gi...@apache.org>.
brusdev commented on a change in pull request #3029: ARTEMIS-2664 Fix the credits acquiring.
URL: https://github.com/apache/activemq-artemis/pull/3029#discussion_r397178617
##########
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:
I replaced the AtomicInteger with an int. Indeed deliveredAcks is used only by
the acknowledge method and it is only executed by the EpollEventLoop thread
bounded with the relative connection channel.
----------------------------------------------------------------
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
[GitHub] [activemq-artemis] franz1981 commented on a change in pull request
#3029: ARTEMIS-2664 Fix the credits acquiring.
Posted by GitBox <gi...@apache.org>.
franz1981 commented on a change in pull request #3029: ARTEMIS-2664 Fix the credits acquiring.
URL: https://github.com/apache/activemq-artemis/pull/3029#discussion_r395207821
##########
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:
@michaelandrepearce FYI is @brusdev himself that has used AtomicInteger the first time there, that's why I insist that is ok to simplify it with the right threading model assumption...
----------------------------------------------------------------
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
[GitHub] [activemq-artemis] brusdev commented on a change in pull request
#3029: ARTEMIS-2664 Fix the delivered acks decrement.
Posted by GitBox <gi...@apache.org>.
brusdev commented on a change in pull request #3029: ARTEMIS-2664 Fix the delivered acks decrement.
URL: https://github.com/apache/activemq-artemis/pull/3029#discussion_r394457065
##########
File path: artemis-protocols/artemis-openwire-protocol/src/main/java/org/apache/activemq/artemis/core/protocol/openwire/amq/AMQConsumer.java
##########
@@ -306,18 +306,15 @@ 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();
- }
-
+ if (deliveredAcks < ackList.size()) {
acquireCredit(ackList.size() - deliveredAcks);
-
- return 0;
- });
+ deliveredAcks = 0;
+ } else {
+ deliveredAcks -= ackList.size();
+ }
} else {
if (ack.isDeliveredAck()) {
- this.deliveredAcks.addAndGet(ack.getMessageCount());
+ this.deliveredAcks += ack.getMessageCount();
Review comment:
I reverted the AtomicInteger. I did not think the `acknowledge` method requires to be thread-safe.
----------------------------------------------------------------
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