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 2021/09/13 10:50:23 UTC
[GitHub] [activemq-artemis] gtully opened a new pull request #3742: ARTEMIS-2007 - refactor to make use of existing refCountForConsumers …
gtully opened a new pull request #3742:
URL: https://github.com/apache/activemq-artemis/pull/3742
…for tracking consumer count and remove need for volatile redistributor
--
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
[GitHub] [activemq-artemis] gtully commented on a change in pull request #3742: ARTEMIS-2007 - refactor to make use of existing refCountForConsumers …
Posted by GitBox <gi...@apache.org>.
gtully commented on a change in pull request #3742:
URL: https://github.com/apache/activemq-artemis/pull/3742#discussion_r709202542
##########
File path: artemis-server/src/main/java/org/apache/activemq/artemis/core/server/impl/QueueImpl.java
##########
@@ -3082,9 +3055,7 @@ private boolean deliver() {
numNoMatch = 0;
numAttempts = 0;
- if (consumer != redistributor) {
- ref = handleMessageGroup(ref, consumer, groupConsumer, groupID);
- }
+ ref = handleMessageGroup(ref, consumer, groupConsumer, groupID);
Review comment:
ok, the reset() is necessary to have any consumer get a second message or try the next message in its iterator. If there are 6 messages pending and the first does not match, unless we reset, that consumer does not get to check the other 5 till the next delivery attempt. With the reset, it is a candidate for another immediate loop.
--
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
[GitHub] [activemq-artemis] gtully commented on a change in pull request #3742: ARTEMIS-2007 - refactor to make use of existing refCountForConsumers …
Posted by GitBox <gi...@apache.org>.
gtully commented on a change in pull request #3742:
URL: https://github.com/apache/activemq-artemis/pull/3742#discussion_r709211974
##########
File path: artemis-server/src/main/java/org/apache/activemq/artemis/core/server/impl/QueueImpl.java
##########
@@ -218,17 +218,17 @@ private void checkIDSupplier(NodeStore<MessageReference> nodeStore) {
protected final ScheduledDeliveryHandler scheduledDeliveryHandler;
- private AtomicLong messagesAdded = new AtomicLong(0);
+ private final AtomicLong messagesAdded = new AtomicLong(0);
- private AtomicLong messagesAcknowledged = new AtomicLong(0);
+ private final AtomicLong messagesAcknowledged = new AtomicLong(0);
- private AtomicLong ackAttempts = new AtomicLong(0);
+ private final AtomicLong ackAttempts = new AtomicLong(0);
Review comment:
in hindsight I should not have touched the finals! but for sure that can be another PR
--
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
[GitHub] [activemq-artemis] gemmellr commented on a change in pull request #3742: ARTEMIS-2007 - refactor to make use of existing refCountForConsumers …
Posted by GitBox <gi...@apache.org>.
gemmellr commented on a change in pull request #3742:
URL: https://github.com/apache/activemq-artemis/pull/3742#discussion_r708099362
##########
File path: artemis-server/src/main/java/org/apache/activemq/artemis/core/server/impl/QueueImpl.java
##########
@@ -3082,9 +3055,7 @@ private boolean deliver() {
numNoMatch = 0;
numAttempts = 0;
- if (consumer != redistributor) {
- ref = handleMessageGroup(ref, consumer, groupConsumer, groupID);
- }
+ ref = handleMessageGroup(ref, consumer, groupConsumer, groupID);
Review comment:
Not particularly no. Neither does the one for 'HANDLED' entirely. Both seem to suggest it will always prefer earlier consumers until they arent 'busy enough' to let it try others. Though it wont then try the others if there is a 'NO_MATCH', it will try the earlier ones again even if still busy.
Looking over it several times now, there is at least the fact that adding a consumer always cancels the redistributor during the process, removing it before adding the new consumer to the consumer list, so the redistributor shouldnt ever end up in the middle of the list...so the reset in the redistributor case shouldn't really be different than hitting the end of the list.
The only other thing I do see for the non-distributor case is that you apparently need to call reset to see updates to the consumer list (such as the above new consumer), which can change as the loop is processing due to the fact it goes in and out of the synchronization. Kind of feels like the obvious point for that is at the end of the existing entries though rather on the first no-match.
--
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
[GitHub] [activemq-artemis] michaelandrepearce commented on a change in pull request #3742: ARTEMIS-2007 - refactor to make use of existing refCountForConsumers …
Posted by GitBox <gi...@apache.org>.
michaelandrepearce commented on a change in pull request #3742:
URL: https://github.com/apache/activemq-artemis/pull/3742#discussion_r707327963
##########
File path: artemis-server/src/main/java/org/apache/activemq/artemis/core/server/impl/QueueImpl.java
##########
@@ -1572,18 +1556,7 @@ public synchronized void cancelRedistributor() {
@Override
public int getConsumerCount() {
- // we don't want to count the redistributor, it is an internal transient entry in the consumer list
- if (redistributor != null) {
- synchronized (this) {
- final int size = consumers.size();
- if (size > 0 && redistributor != null) {
- return size - 1;
- } else {
- return size;
- }
- }
- }
- return consumers.size();
+ return refCountForConsumers.getCount();
Review comment:
e.g. what happens if someone adds new code to add something into consumer collection, but doesnt remember to update this other ref counter thing.
--
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
[GitHub] [activemq-artemis] gtully commented on a change in pull request #3742: ARTEMIS-2007 - refactor to make use of existing refCountForConsumers …
Posted by GitBox <gi...@apache.org>.
gtully commented on a change in pull request #3742:
URL: https://github.com/apache/activemq-artemis/pull/3742#discussion_r707493306
##########
File path: artemis-server/src/main/java/org/apache/activemq/artemis/core/server/impl/QueueImpl.java
##########
@@ -179,7 +179,7 @@
protected final PageSubscription pageSubscription;
- private ReferenceCounter refCountForConsumers;
+ protected final ReferenceCounter refCountForConsumers;
Review comment:
true, that is not necessary. will fix. thanks!
--
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
[GitHub] [activemq-artemis] gemmellr commented on a change in pull request #3742: ARTEMIS-2007 - refactor to make use of existing refCountForConsumers …
Posted by GitBox <gi...@apache.org>.
gemmellr commented on a change in pull request #3742:
URL: https://github.com/apache/activemq-artemis/pull/3742#discussion_r709272118
##########
File path: artemis-server/src/main/java/org/apache/activemq/artemis/core/server/impl/QueueImpl.java
##########
@@ -3082,9 +3055,7 @@ private boolean deliver() {
numNoMatch = 0;
numAttempts = 0;
- if (consumer != redistributor) {
- ref = handleMessageGroup(ref, consumer, groupConsumer, groupID);
- }
+ ref = handleMessageGroup(ref, consumer, groupConsumer, groupID);
Review comment:
Javadoc could probably do with an update to describe the actual behaviour then if thats the case: reset() "Resets the iterator so you can re-iterate over all elements."
Last I looked at it it seemed to me like it would reset the cursor (especially if there was a new consumer added before, since it replaced the underlying iterator entirely with a new one), which is what I would expect a 'reset' method to do. There is a separate 'repeat()' I think that I would expect to do what you just described.
--
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
[GitHub] [activemq-artemis] gtully commented on a change in pull request #3742: ARTEMIS-2007 - refactor to make use of existing refCountForConsumers …
Posted by GitBox <gi...@apache.org>.
gtully commented on a change in pull request #3742:
URL: https://github.com/apache/activemq-artemis/pull/3742#discussion_r707467913
##########
File path: artemis-server/src/main/java/org/apache/activemq/artemis/core/server/impl/QueueImpl.java
##########
@@ -3082,9 +3055,7 @@ private boolean deliver() {
numNoMatch = 0;
numAttempts = 0;
- if (consumer != redistributor) {
- ref = handleMessageGroup(ref, consumer, groupConsumer, groupID);
- }
+ ref = handleMessageGroup(ref, consumer, groupConsumer, groupID);
Review comment:
that != was always false, it was comparing different types in error. has been that way seems since the beginning. the redistributor has logic to reply busy when a message has groups, so it will never handle a group message. the handle returns the ref if there are no group bits in play so it is safe to always call 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.
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] gtully commented on a change in pull request #3742: ARTEMIS-2007 - refactor to make use of existing refCountForConsumers …
Posted by GitBox <gi...@apache.org>.
gtully commented on a change in pull request #3742:
URL: https://github.com/apache/activemq-artemis/pull/3742#discussion_r714954724
##########
File path: artemis-server/src/main/java/org/apache/activemq/artemis/core/server/impl/QueueImpl.java
##########
@@ -218,17 +218,17 @@ private void checkIDSupplier(NodeStore<MessageReference> nodeStore) {
protected final ScheduledDeliveryHandler scheduledDeliveryHandler;
- private AtomicLong messagesAdded = new AtomicLong(0);
+ private final AtomicLong messagesAdded = new AtomicLong(0);
Review comment:
am going to leave that to a separate PR and investigate AtomicLongFieldUpdater
--
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
[GitHub] [activemq-artemis] gtully commented on a change in pull request #3742: ARTEMIS-2007 - refactor to make use of existing refCountForConsumers …
Posted by GitBox <gi...@apache.org>.
gtully commented on a change in pull request #3742:
URL: https://github.com/apache/activemq-artemis/pull/3742#discussion_r714979533
##########
File path: artemis-server/src/main/java/org/apache/activemq/artemis/core/server/impl/QueueImpl.java
##########
@@ -3082,9 +3055,7 @@ private boolean deliver() {
numNoMatch = 0;
numAttempts = 0;
- if (consumer != redistributor) {
- ref = handleMessageGroup(ref, consumer, groupConsumer, groupID);
- }
+ ref = handleMessageGroup(ref, consumer, groupConsumer, groupID);
Review comment:
I have updated the javadoc. the reset - will pick up changes, but will continue iteration with a restart from the current point, essentially making the collection circular.
the the repeat just replays the last element.
--
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
[GitHub] [activemq-artemis] gemmellr commented on a change in pull request #3742: ARTEMIS-2007 - refactor to make use of existing refCountForConsumers …
Posted by GitBox <gi...@apache.org>.
gemmellr commented on a change in pull request #3742:
URL: https://github.com/apache/activemq-artemis/pull/3742#discussion_r707443939
##########
File path: artemis-server/src/main/java/org/apache/activemq/artemis/core/server/impl/QueueImpl.java
##########
@@ -3082,9 +3055,7 @@ private boolean deliver() {
numNoMatch = 0;
numAttempts = 0;
- if (consumer != redistributor) {
- ref = handleMessageGroup(ref, consumer, groupConsumer, groupID);
- }
+ ref = handleMessageGroup(ref, consumer, groupConsumer, groupID);
Review comment:
If the redistributor now handles groups and didnt before, does it need to do anything extra when it is cancelled that it wasnt doing before (since it didnt need to) that e.g other consumers currently do when removed?
##########
File path: artemis-server/src/main/java/org/apache/activemq/artemis/core/server/impl/QueueImpl.java
##########
@@ -218,17 +218,17 @@ private void checkIDSupplier(NodeStore<MessageReference> nodeStore) {
protected final ScheduledDeliveryHandler scheduledDeliveryHandler;
- private AtomicLong messagesAdded = new AtomicLong(0);
+ private final AtomicLong messagesAdded = new AtomicLong(0);
- private AtomicLong messagesAcknowledged = new AtomicLong(0);
+ private final AtomicLong messagesAcknowledged = new AtomicLong(0);
- private AtomicLong ackAttempts = new AtomicLong(0);
+ private final AtomicLong ackAttempts = new AtomicLong(0);
Review comment:
It should certainly be in a separate commit if it is done, it has nothing to do with this 'refactoring' (arguably neither do the finals).
##########
File path: artemis-server/src/main/java/org/apache/activemq/artemis/core/server/impl/QueueImpl.java
##########
@@ -179,7 +179,7 @@
protected final PageSubscription pageSubscription;
- private ReferenceCounter refCountForConsumers;
+ protected final ReferenceCounter refCountForConsumers;
Review comment:
Why 'protected'? It was private before and doesnt seem to be used elsewhere now, and there was/is still a getter if it is.
--
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
[GitHub] [activemq-artemis] gtully commented on a change in pull request #3742: ARTEMIS-2007 - refactor to make use of existing refCountForConsumers …
Posted by GitBox <gi...@apache.org>.
gtully commented on a change in pull request #3742:
URL: https://github.com/apache/activemq-artemis/pull/3742#discussion_r707470239
##########
File path: artemis-server/src/main/java/org/apache/activemq/artemis/core/server/impl/QueueImpl.java
##########
@@ -3082,9 +3055,7 @@ private boolean deliver() {
numNoMatch = 0;
numAttempts = 0;
- if (consumer != redistributor) {
- ref = handleMessageGroup(ref, consumer, groupConsumer, groupID);
- }
+ ref = handleMessageGroup(ref, consumer, groupConsumer, groupID);
Review comment:
it did handle groups 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.
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] gemmellr commented on a change in pull request #3742: ARTEMIS-2007 - refactor to make use of existing refCountForConsumers …
Posted by GitBox <gi...@apache.org>.
gemmellr commented on a change in pull request #3742:
URL: https://github.com/apache/activemq-artemis/pull/3742#discussion_r715014923
##########
File path: artemis-server/src/main/java/org/apache/activemq/artemis/core/server/impl/QueueImpl.java
##########
@@ -3082,9 +3055,7 @@ private boolean deliver() {
numNoMatch = 0;
numAttempts = 0;
- if (consumer != redistributor) {
- ref = handleMessageGroup(ref, consumer, groupConsumer, groupID);
- }
+ ref = handleMessageGroup(ref, consumer, groupConsumer, groupID);
Review comment:
It didnt seem like it would continue from the current point (especially if there were changes, when it replaces the underlying iterator) since it implements the reset() with a "moveTo(-1)".
But, on closer inspection, it seems to do a mixture of things - it apparently _does_ reset the cursor to the start as I thought due to the above, but that cursor is only in terms of 'the highest priority' sub-iterator. However it _does not_ reset the cursor within that particular sub-iterator to the start (except perhaps when it has changed, when I guess it will thanks to replacing the iterators). The stuff going on here so far isnt hitting the priority variation, so it isnt seeing the distinction, so only seems to behave like you described so far.
In short, I dont think it works exactly like either of us think it does, and I'm thinking that QueueConsumersImplTest is lacking some tests to verify whatever its actually meant to be doing overall.
--
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
[GitHub] [activemq-artemis] franz1981 commented on a change in pull request #3742: ARTEMIS-2007 - refactor to make use of existing refCountForConsumers …
Posted by GitBox <gi...@apache.org>.
franz1981 commented on a change in pull request #3742:
URL: https://github.com/apache/activemq-artemis/pull/3742#discussion_r707287160
##########
File path: artemis-server/src/main/java/org/apache/activemq/artemis/core/server/impl/QueueImpl.java
##########
@@ -218,17 +218,17 @@ private void checkIDSupplier(NodeStore<MessageReference> nodeStore) {
protected final ScheduledDeliveryHandler scheduledDeliveryHandler;
- private AtomicLong messagesAdded = new AtomicLong(0);
+ private final AtomicLong messagesAdded = new AtomicLong(0);
Review comment:
Sometime we use AtomicLongFieldUpdater and sometime we use AtomicLong. This could be the chance to unify the way we treat counters like these, chosing a single tool for the job...
IMO if these counters are not used for logic, but for telemetry, probably a `LongAdder` (or some of the JCTools counters) would work much better then `AtomicLong/AtomicLongFieldUpdater`. because `LongAdder` scales way better and cost must less for the thread that increment it, especially if is happening in the hot path
--
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
[GitHub] [activemq-artemis] clebertsuconic commented on a change in pull request #3742: ARTEMIS-2007 - refactor to make use of existing refCountForConsumers …
Posted by GitBox <gi...@apache.org>.
clebertsuconic commented on a change in pull request #3742:
URL: https://github.com/apache/activemq-artemis/pull/3742#discussion_r707297929
##########
File path: artemis-server/src/main/java/org/apache/activemq/artemis/core/server/impl/QueueImpl.java
##########
@@ -218,17 +218,17 @@ private void checkIDSupplier(NodeStore<MessageReference> nodeStore) {
protected final ScheduledDeliveryHandler scheduledDeliveryHandler;
- private AtomicLong messagesAdded = new AtomicLong(0);
+ private final AtomicLong messagesAdded = new AtomicLong(0);
Review comment:
QueueImpl is a heavy object.. I agree the FieldUPdater would be better on this case. it would help make it lighter.
--
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
[GitHub] [activemq-artemis] gemmellr commented on a change in pull request #3742: ARTEMIS-2007 - refactor to make use of existing refCountForConsumers …
Posted by GitBox <gi...@apache.org>.
gemmellr commented on a change in pull request #3742:
URL: https://github.com/apache/activemq-artemis/pull/3742#discussion_r715014923
##########
File path: artemis-server/src/main/java/org/apache/activemq/artemis/core/server/impl/QueueImpl.java
##########
@@ -3082,9 +3055,7 @@ private boolean deliver() {
numNoMatch = 0;
numAttempts = 0;
- if (consumer != redistributor) {
- ref = handleMessageGroup(ref, consumer, groupConsumer, groupID);
- }
+ ref = handleMessageGroup(ref, consumer, groupConsumer, groupID);
Review comment:
It didnt seem like it would continue from the current point (especially if there were changes, when it replaces the underlying iterator) since it implements the reset() with a "moveTo(-1)".
But, on closer inspection, it seems to do a mixture of things - it apparently _does_ reset the cursor to the start as I thought due to the above, but only in terms of 'the highest priority' sub-iterator. However it _does not_ reset the cursor within that particular sub-iterator to the start (except perhaps when it has changed, when I guess it will thanks to replacing the iterators). The stuff going on here so far isnt hitting the priority variation, so it isnt seeing the distinction.
In short, I dont think it does exactly what either of us think it does, and I'm thinking that QueueConsumersImplTest is lacking some tests to verify whatever its actually meant to be doing overall.
--
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
[GitHub] [activemq-artemis] clebertsuconic commented on a change in pull request #3742: ARTEMIS-2007 - refactor to make use of existing refCountForConsumers …
Posted by GitBox <gi...@apache.org>.
clebertsuconic commented on a change in pull request #3742:
URL: https://github.com/apache/activemq-artemis/pull/3742#discussion_r707298405
##########
File path: artemis-server/src/main/java/org/apache/activemq/artemis/core/server/impl/QueueImpl.java
##########
@@ -218,17 +218,17 @@ private void checkIDSupplier(NodeStore<MessageReference> nodeStore) {
protected final ScheduledDeliveryHandler scheduledDeliveryHandler;
- private AtomicLong messagesAdded = new AtomicLong(0);
+ private final AtomicLong messagesAdded = new AtomicLong(0);
- private AtomicLong messagesAcknowledged = new AtomicLong(0);
+ private final AtomicLong messagesAcknowledged = new AtomicLong(0);
- private AtomicLong ackAttempts = new AtomicLong(0);
+ private final AtomicLong ackAttempts = new AtomicLong(0);
Review comment:
hmmm... not part of your change.. but if you could improve it with the FieldUpdater :)
--
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
[GitHub] [activemq-artemis] gemmellr commented on a change in pull request #3742: ARTEMIS-2007 - refactor to make use of existing refCountForConsumers …
Posted by GitBox <gi...@apache.org>.
gemmellr commented on a change in pull request #3742:
URL: https://github.com/apache/activemq-artemis/pull/3742#discussion_r707500849
##########
File path: artemis-server/src/main/java/org/apache/activemq/artemis/core/server/impl/QueueImpl.java
##########
@@ -3082,9 +3055,7 @@ private boolean deliver() {
numNoMatch = 0;
numAttempts = 0;
- if (consumer != redistributor) {
- ref = handleMessageGroup(ref, consumer, groupConsumer, groupID);
- }
+ ref = handleMessageGroup(ref, consumer, groupConsumer, groupID);
Review comment:
Seems the incorrect typed check went in with the previous commit in place of a null check looking to similarly protect it:
https://github.com/apache/activemq-artemis/commit/224b89810d023e3ee73b014a48585da8f6048237#diff-fc4a9a0dc01f79ecc1435246dc85446fa57d6d04e1dffed53c522d8c145623fdR3085
But yep, didnt know that the redistributor handle check returns NO_MATCH (it only returns 'busy' if not active), so it never got to either of those old checks anyway (though by returning 'NO_MATCH' it will reset the consumers iterator, meaning any consumer after it in the list isnt getting tried until it goes away...).
--
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
[GitHub] [activemq-artemis] gemmellr commented on a change in pull request #3742: ARTEMIS-2007 - refactor to make use of existing refCountForConsumers …
Posted by GitBox <gi...@apache.org>.
gemmellr commented on a change in pull request #3742:
URL: https://github.com/apache/activemq-artemis/pull/3742#discussion_r708099362
##########
File path: artemis-server/src/main/java/org/apache/activemq/artemis/core/server/impl/QueueImpl.java
##########
@@ -3082,9 +3055,7 @@ private boolean deliver() {
numNoMatch = 0;
numAttempts = 0;
- if (consumer != redistributor) {
- ref = handleMessageGroup(ref, consumer, groupConsumer, groupID);
- }
+ ref = handleMessageGroup(ref, consumer, groupConsumer, groupID);
Review comment:
Not particularly no. Neither does the one for 'HANDLED' entirely. Both seem to suggest it will always prefer earlier consumers until they arent 'busy enough' to let it try others. Though it wont then try the others if there is a 'NO_MATCH', it will try the earlier ones again even if still busy.
Looking over it several times now, there is at least the fact that adding a consumer always cancels the redistributor during the process, before adding it to the consumer list, so the redistributor shouldnt ever end up in the middle of the list...so the reset in the redistributor case shouldn't really be different than hitting the end of the list.
The only other thing I do see for the non-distributor case is that you apparently need to call reset to see updates to the consumer list, which can change as the loop is processing due to the fact it goes in and out of the synchronization. Kind of feels like the obvious point for that is at the end of the existing entries though rather on the first no-match.
--
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
[GitHub] [activemq-artemis] gtully merged pull request #3742: ARTEMIS-2007 - refactor to make use of existing refCountForConsumers …
Posted by GitBox <gi...@apache.org>.
gtully merged pull request #3742:
URL: https://github.com/apache/activemq-artemis/pull/3742
--
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
[GitHub] [activemq-artemis] clebertsuconic commented on a change in pull request #3742: ARTEMIS-2007 - refactor to make use of existing refCountForConsumers …
Posted by GitBox <gi...@apache.org>.
clebertsuconic commented on a change in pull request #3742:
URL: https://github.com/apache/activemq-artemis/pull/3742#discussion_r707300551
##########
File path: artemis-server/src/main/java/org/apache/activemq/artemis/core/server/impl/QueueImpl.java
##########
@@ -218,17 +218,17 @@ private void checkIDSupplier(NodeStore<MessageReference> nodeStore) {
protected final ScheduledDeliveryHandler scheduledDeliveryHandler;
- private AtomicLong messagesAdded = new AtomicLong(0);
+ private final AtomicLong messagesAdded = new AtomicLong(0);
- private AtomicLong messagesAcknowledged = new AtomicLong(0);
+ private final AtomicLong messagesAcknowledged = new AtomicLong(0);
- private AtomicLong ackAttempts = new AtomicLong(0);
+ private final AtomicLong ackAttempts = new AtomicLong(0);
Review comment:
the FieldUpdater thing I asked is a wish... not mandatory for your PR... if you can do it.. great... otherwise we can make it another PR for that.
--
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
[GitHub] [activemq-artemis] gtully commented on a change in pull request #3742: ARTEMIS-2007 - refactor to make use of existing refCountForConsumers …
Posted by GitBox <gi...@apache.org>.
gtully commented on a change in pull request #3742:
URL: https://github.com/apache/activemq-artemis/pull/3742#discussion_r707510310
##########
File path: artemis-server/src/main/java/org/apache/activemq/artemis/core/server/impl/QueueImpl.java
##########
@@ -3082,9 +3055,7 @@ private boolean deliver() {
numNoMatch = 0;
numAttempts = 0;
- if (consumer != redistributor) {
- ref = handleMessageGroup(ref, consumer, groupConsumer, groupID);
- }
+ ref = handleMessageGroup(ref, consumer, groupConsumer, groupID);
Review comment:
the reset on NO_MATCH I am not understanding. Does that make sense to you?
--
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
[GitHub] [activemq-artemis] gtully commented on a change in pull request #3742: ARTEMIS-2007 - refactor to make use of existing refCountForConsumers …
Posted by GitBox <gi...@apache.org>.
gtully commented on a change in pull request #3742:
URL: https://github.com/apache/activemq-artemis/pull/3742#discussion_r707374180
##########
File path: artemis-server/src/main/java/org/apache/activemq/artemis/core/server/impl/QueueImpl.java
##########
@@ -1572,18 +1556,7 @@ public synchronized void cancelRedistributor() {
@Override
public int getConsumerCount() {
- // we don't want to count the redistributor, it is an internal transient entry in the consumer list
- if (redistributor != null) {
- synchronized (this) {
- final int size = consumers.size();
- if (size > 0 && redistributor != null) {
- return size - 1;
- } else {
- return size;
- }
- }
- }
- return consumers.size();
+ return refCountForConsumers.getCount();
Review comment:
the point is there are two different counts, now that consumers has a redistributor that is internal. The need to keep then in sync is not new and it is confined to add/remove consumers
--
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
[GitHub] [activemq-artemis] michaelandrepearce commented on a change in pull request #3742: ARTEMIS-2007 - refactor to make use of existing refCountForConsumers …
Posted by GitBox <gi...@apache.org>.
michaelandrepearce commented on a change in pull request #3742:
URL: https://github.com/apache/activemq-artemis/pull/3742#discussion_r707327211
##########
File path: artemis-server/src/main/java/org/apache/activemq/artemis/core/server/impl/QueueImpl.java
##########
@@ -1572,18 +1556,7 @@ public synchronized void cancelRedistributor() {
@Override
public int getConsumerCount() {
- // we don't want to count the redistributor, it is an internal transient entry in the consumer list
- if (redistributor != null) {
- synchronized (this) {
- final int size = consumers.size();
- if (size > 0 && redistributor != null) {
- return size - 1;
- } else {
- return size;
- }
- }
- }
- return consumers.size();
+ return refCountForConsumers.getCount();
Review comment:
I dont like this tbh, you have a collection with is consumers, should just use its value, else we end up having issues of one size thing being different from another.
--
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
[GitHub] [activemq-artemis] gtully commented on a change in pull request #3742: ARTEMIS-2007 - refactor to make use of existing refCountForConsumers …
Posted by GitBox <gi...@apache.org>.
gtully commented on a change in pull request #3742:
URL: https://github.com/apache/activemq-artemis/pull/3742#discussion_r709211088
##########
File path: artemis-server/src/main/java/org/apache/activemq/artemis/core/server/impl/QueueImpl.java
##########
@@ -3082,9 +3055,7 @@ private boolean deliver() {
numNoMatch = 0;
numAttempts = 0;
- if (consumer != redistributor) {
- ref = handleMessageGroup(ref, consumer, groupConsumer, groupID);
- }
+ ref = handleMessageGroup(ref, consumer, groupConsumer, groupID);
Review comment:
of note: the reset() does not put the cursor back to the start, rather it moves the end such that the current element gets another go. The iterator is circular if reset is called. In this way, reset does not skip the next element.
--
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
[GitHub] [activemq-artemis] gtully commented on a change in pull request #3742: ARTEMIS-2007 - refactor to make use of existing refCountForConsumers …
Posted by GitBox <gi...@apache.org>.
gtully commented on a change in pull request #3742:
URL: https://github.com/apache/activemq-artemis/pull/3742#discussion_r714979533
##########
File path: artemis-server/src/main/java/org/apache/activemq/artemis/core/server/impl/QueueImpl.java
##########
@@ -3082,9 +3055,7 @@ private boolean deliver() {
numNoMatch = 0;
numAttempts = 0;
- if (consumer != redistributor) {
- ref = handleMessageGroup(ref, consumer, groupConsumer, groupID);
- }
+ ref = handleMessageGroup(ref, consumer, groupConsumer, groupID);
Review comment:
I have updated the javadoc. the reset - will pick up changes, but will continue iteration with a restart from the current point, essentially making the collection circular.
The repeat, just replays the last element.
--
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