You are viewing a plain text version of this content. The canonical link for it is here.
Posted to gitbox@activemq.apache.org by "gtully (via GitHub)" <gi...@apache.org> on 2023/09/06 16:53:09 UTC
[GitHub] [activemq-artemis] gtully opened a new pull request, #4606: ARTEMIS-4418 use consumer delivery sequence in place message id for o…
gtully opened a new pull request, #4606:
URL: https://github.com/apache/activemq-artemis/pull/4606
…penwire broker sequence id, makes delivery ccount calculation independent of message order
--
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 diff in pull request #4606: ARTEMIS-4418 use consumer delivery sequence in place message id for o…
Posted by "gtully (via GitHub)" <gi...@apache.org>.
gtully commented on code in PR #4606:
URL: https://github.com/apache/activemq-artemis/pull/4606#discussion_r1318510871
##########
artemis-server/src/main/java/org/apache/activemq/artemis/core/server/impl/ServerConsumerImpl.java:
##########
@@ -656,6 +656,7 @@ public void removeItself() throws Exception {
browserDeliverer.close();
} else {
messageQueue.removeConsumer(this);
+ messageQueue.deliverAsync();
Review Comment:
it could be but the test that now has an exclusive and non exclusive variant was intermittently failing and this was the 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.
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 #4606: ARTEMIS-4418 use consumer delivery sequence in place message id for o…
Posted by "gtully (via GitHub)" <gi...@apache.org>.
gtully merged PR #4606:
URL: https://github.com/apache/activemq-artemis/pull/4606
--
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 diff in pull request #4606: ARTEMIS-4418 use consumer delivery sequence in place message id for o…
Posted by "gtully (via GitHub)" <gi...@apache.org>.
gtully commented on code in PR #4606:
URL: https://github.com/apache/activemq-artemis/pull/4606#discussion_r1318410499
##########
artemis-protocols/artemis-openwire-protocol/src/main/java/org/apache/activemq/artemis/core/protocol/openwire/amq/AMQConsumer.java:
##########
@@ -81,6 +81,7 @@ public class AMQConsumer {
private boolean internalAddress = false;
private volatile Set<MessageReference> rolledbackMessageRefs;
private ScheduledFuture<?> delayedDispatchPrompter;
+ private long deliveredSequenceId = 0;
Review Comment:
correction, thread safe for the increment, so 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.
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 diff in pull request #4606: ARTEMIS-4418 use consumer delivery sequence in place message id for o…
Posted by "gemmellr (via GitHub)" <gi...@apache.org>.
gemmellr commented on code in PR #4606:
URL: https://github.com/apache/activemq-artemis/pull/4606#discussion_r1318754079
##########
artemis-server/src/main/java/org/apache/activemq/artemis/core/server/impl/ServerConsumerImpl.java:
##########
@@ -656,6 +656,7 @@ public void removeItself() throws Exception {
browserDeliverer.close();
} else {
messageQueue.removeConsumer(this);
+ messageQueue.deliverAsync();
Review Comment:
sure, but that seems more like a bug in the queue itself, and this change isnt specific to openwire, should have been addressed seperately from this openwire-consumer-specific JIRA.
--
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 diff in pull request #4606: ARTEMIS-4418 use consumer delivery sequence in place message id for o…
Posted by "gtully (via GitHub)" <gi...@apache.org>.
gtully commented on code in PR #4606:
URL: https://github.com/apache/activemq-artemis/pull/4606#discussion_r1318523331
##########
artemis-protocols/artemis-openwire-protocol/src/main/java/org/apache/activemq/artemis/core/protocol/openwire/amq/AMQConsumer.java:
##########
@@ -293,6 +295,7 @@ public int handleDeliver(MessageReference reference, ICoreMessage message) {
}
//handleDeliver is performed by an executor (see JBPAPP-6030): any AMQConsumer can share the session.wireFormat()
dispatch = OpenWireMessageConverter.createMessageDispatch(reference, message, session.wireFormat(), this, session.getCoreServer().getNodeManager().getUUID());
+ dispatch.getMessage().getMessageId().setBrokerSequenceId(deliveredSequenceId.getAndAdd(1));
Review Comment:
good idea, there will be more impact by that api change but it is better. 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 diff in pull request #4606: ARTEMIS-4418 use consumer delivery sequence in place message id for o…
Posted by "gemmellr (via GitHub)" <gi...@apache.org>.
gemmellr commented on code in PR #4606:
URL: https://github.com/apache/activemq-artemis/pull/4606#discussion_r1318474790
##########
artemis-protocols/artemis-openwire-protocol/src/main/java/org/apache/activemq/artemis/core/protocol/openwire/amq/AMQConsumer.java:
##########
@@ -293,6 +295,7 @@ public int handleDeliver(MessageReference reference, ICoreMessage message) {
}
//handleDeliver is performed by an executor (see JBPAPP-6030): any AMQConsumer can share the session.wireFormat()
dispatch = OpenWireMessageConverter.createMessageDispatch(reference, message, session.wireFormat(), this, session.getCoreServer().getNodeManager().getUUID());
+ dispatch.getMessage().getMessageId().setBrokerSequenceId(deliveredSequenceId.getAndAdd(1));
Review Comment:
The createMessageDispatch method already looks to set this, to the existing different value...perhaps it should be changed to pass this value in there to set, helping avoid any inconsistency later.
Also getAndAdd - > getAndIncrement ?
##########
artemis-server/src/main/java/org/apache/activemq/artemis/core/server/impl/ServerConsumerImpl.java:
##########
@@ -656,6 +656,7 @@ public void removeItself() throws Exception {
browserDeliverer.close();
} else {
messageQueue.removeConsumer(this);
+ messageQueue.deliverAsync();
Review Comment:
Kind of wonder if it shouldnt be the queue doing this instead of the consumer? Also feels like it may be a separate change really.
--
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 diff in pull request #4606: ARTEMIS-4418 use consumer delivery sequence in place message id for o…
Posted by "gtully (via GitHub)" <gi...@apache.org>.
gtully commented on code in PR #4606:
URL: https://github.com/apache/activemq-artemis/pull/4606#discussion_r1318397553
##########
artemis-protocols/artemis-openwire-protocol/src/main/java/org/apache/activemq/artemis/core/protocol/openwire/amq/AMQConsumer.java:
##########
@@ -81,6 +81,7 @@ public class AMQConsumer {
private boolean internalAddress = false;
private volatile Set<MessageReference> rolledbackMessageRefs;
private ScheduledFuture<?> delayedDispatchPrompter;
+ private long deliveredSequenceId = 0;
Review Comment:
this needs to be volatile, in the case that the consumer referenced more than one queue, or even in the case that a queue executor is pooled.
--
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