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/13 18:45:20 UTC
[GitHub] [activemq-artemis] jbertram opened a new pull request #3017:
ARTEMIS-2649 refactor ORIG message props
jbertram opened a new pull request #3017: ARTEMIS-2649 refactor ORIG message props
URL: https://github.com/apache/activemq-artemis/pull/3017
ORIG message propertes like _AMQ_ORIG_ADDRESS are added to messages
during various broker operations (e.g. diverting a message, expiring a
message, etc.). However, if multiple operations set these properties on
the same message (e.g. administratively moving a message which
eventually gets sent to a dead-letter address) then important details
can be lost. This is particularly problematic when using auto-created
dead-letter or exipry resources which use filters based on
_AMQ_ORIG_ADDRESS and can lead to message loss.
This commit implements "breadcrumb" properties so that the full history
of the message is retained and the most recent information is stored in
the original ORIG properties.
----------------------------------------------------------------
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] jbertram commented on a change in pull request
#3017: ARTEMIS-2649 refactor ORIG message props
Posted by GitBox <gi...@apache.org>.
jbertram commented on a change in pull request #3017: ARTEMIS-2649 refactor ORIG message props
URL: https://github.com/apache/activemq-artemis/pull/3017#discussion_r396570384
##########
File path: artemis-server/src/main/java/org/apache/activemq/artemis/core/server/impl/QueueImpl.java
##########
@@ -3418,7 +3419,12 @@ private boolean sendToDeadLetterAddress(final Transaction tx,
ref.acknowledge(tx, AckReason.KILLED, null);
} else {
ActiveMQServerLogger.LOGGER.messageExceededMaxDeliverySendtoDLA(ref, deadLetterAddress, name);
- move(tx, deadLetterAddress, null, ref, false, AckReason.KILLED, null);
+ RoutingStatus status = move(tx, deadLetterAddress, null, ref, false, AckReason.KILLED, null);
+
+ // this shouldn't happen, but in case it does it's better to log a message than just drop the message silently
+ if (status.equals(RoutingStatus.NO_BINDINGS) && server.getAddressSettingsRepository().getMatch(getAddress().toString()).isAutoCreateDeadLetterResources()) {
+ ActiveMQServerLogger.LOGGER.noMatchingBindingsOnDLAWithAutoCreateDLAResources(deadLetterAddress, ref.toString());
Review comment:
Rather than get into a protracted discussion about coding philosophy I'll refer to [my comment on the Jira](https://issues.apache.org/jira/browse/ARTEMIS-2649?focusedCommentId=17058399&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-17058399). Simply put, adding complexity is likely to result in more bugs, not less. Logging a message is extremely simple and follows precedence from other parts of the code when we encounter things that "shouldn't happen." When you add complexity to deal with these types of situations then you inevitably open yourself up to more potential bugs which requires more testing and oftentimes more user-configurable options (because not everybody wants to handle everything the same way) which requires even more code, tests, and documentation. Furthermore, if the original defensive code is sufficiently complex then it will need its own defensive code and that code will need tests, etc. Adding complexity is an untenable solution.
I think the best defense against bugs here is simply more testing (as noted on the Jira). You tested the implementation and found a bug. I added a test for your use-case and fixed the bug. Please continue testing to verify everything works for your expected use-cases.
I have no plans to allow additional customization of the filters for auto-created dead-letter-queues.
----------------------------------------------------------------
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] PiotrKlimczak commented on a change in pull
request #3017: ARTEMIS-2649 refactor ORIG message props
Posted by GitBox <gi...@apache.org>.
PiotrKlimczak commented on a change in pull request #3017: ARTEMIS-2649 refactor ORIG message props
URL: https://github.com/apache/activemq-artemis/pull/3017#discussion_r396073463
##########
File path: artemis-server/src/main/java/org/apache/activemq/artemis/core/server/impl/QueueImpl.java
##########
@@ -3418,7 +3419,12 @@ private boolean sendToDeadLetterAddress(final Transaction tx,
ref.acknowledge(tx, AckReason.KILLED, null);
} else {
ActiveMQServerLogger.LOGGER.messageExceededMaxDeliverySendtoDLA(ref, deadLetterAddress, name);
- move(tx, deadLetterAddress, null, ref, false, AckReason.KILLED, null);
+ RoutingStatus status = move(tx, deadLetterAddress, null, ref, false, AckReason.KILLED, null);
+
+ // this shouldn't happen, but in case it does it's better to log a message than just drop the message silently
+ if (status.equals(RoutingStatus.NO_BINDINGS) && server.getAddressSettingsRepository().getMatch(getAddress().toString()).isAutoCreateDeadLetterResources()) {
+ ActiveMQServerLogger.LOGGER.noMatchingBindingsOnDLAWithAutoCreateDLAResources(deadLetterAddress, ref.toString());
Review comment:
Thanks for your answer.
I couldn't agree more with above in regards to manual configuration of queues and dlqs.
However the same problem falls into much different category when auto-create-dead-letter-resources is used, as then it is Brokers responsibility to take care of everything it and not lose a message in any case- this is my only concern.
However obviously there is potential for loss to happen, as per your warning message.
----------------------------------------------------------------
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] PiotrKlimczak commented on a change in pull
request #3017: ARTEMIS-2649 refactor ORIG message props
Posted by GitBox <gi...@apache.org>.
PiotrKlimczak commented on a change in pull request #3017: ARTEMIS-2649 refactor ORIG message props
URL: https://github.com/apache/activemq-artemis/pull/3017#discussion_r395979696
##########
File path: artemis-core-client/src/main/java/org/apache/activemq/artemis/api/core/Message.java
##########
@@ -463,30 +463,40 @@ default void reencode() {
}
default void referenceOriginalMessage(final Message original, String originalQueue) {
- String queueOnMessage = original.getAnnotationString(Message.HDR_ORIGINAL_QUEUE);
+ String originalQueueOnMessage = original.getAnnotationString(Message.HDR_ORIGINAL_QUEUE);
- if (queueOnMessage != null) {
- setAnnotation(Message.HDR_ORIGINAL_QUEUE, queueOnMessage);
- } else if (originalQueue != null) {
- setAnnotation(Message.HDR_ORIGINAL_QUEUE, originalQueue);
+ if (originalQueueOnMessage != null) {
+ setBreadcrumb(Message.HDR_ORIGINAL_QUEUE, originalQueueOnMessage);
}
+ setAnnotation(Message.HDR_ORIGINAL_QUEUE, originalQueue);
+
Object originalID = original.getAnnotation(Message.HDR_ORIG_MESSAGE_ID);
if (originalID != null) {
Review comment:
The code suggests, that setBreadcrumb might happen for Message.HDR_ORIGINAL_QUEUE but not for Message.HDR_ORIGINAL_ADDRESS and Message.HDR_ORIG_MESSAGE_ID if for example originalID is null.
As mentioned in past, I am no expert in Artemis therefore I cannot judge if such situation would ever happen, however by analysing this code only, there is a chance that breadcrumbs will run out of sync, as they are set under two separate conditions.
Will that be a problem however?
----------------------------------------------------------------
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] jbertram commented on a change in pull request
#3017: ARTEMIS-2649 refactor ORIG message props
Posted by GitBox <gi...@apache.org>.
jbertram commented on a change in pull request #3017: ARTEMIS-2649 refactor ORIG message props
URL: https://github.com/apache/activemq-artemis/pull/3017#discussion_r396022958
##########
File path: artemis-server/src/main/java/org/apache/activemq/artemis/core/server/impl/QueueImpl.java
##########
@@ -3418,7 +3419,12 @@ private boolean sendToDeadLetterAddress(final Transaction tx,
ref.acknowledge(tx, AckReason.KILLED, null);
} else {
ActiveMQServerLogger.LOGGER.messageExceededMaxDeliverySendtoDLA(ref, deadLetterAddress, name);
- move(tx, deadLetterAddress, null, ref, false, AckReason.KILLED, null);
+ RoutingStatus status = move(tx, deadLetterAddress, null, ref, false, AckReason.KILLED, null);
+
+ // this shouldn't happen, but in case it does it's better to log a message than just drop the message silently
+ if (status.equals(RoutingStatus.NO_BINDINGS) && server.getAddressSettingsRepository().getMatch(getAddress().toString()).isAutoCreateDeadLetterResources()) {
+ ActiveMQServerLogger.LOGGER.noMatchingBindingsOnDLAWithAutoCreateDLAResources(deadLetterAddress, ref.toString());
Review comment:
For what it's worth, there's precedence for this (which is why I implemented it this way). Messages are dropped with a warning in the case where `max-delivery-attempts` has been exceeded and there's no `dead-letter-address` configured or if there is a `dead-letter-address` configured but there are no queues bound to it. The same is true in the case where a message expires and there's no `expiry-address` configured or if there is a `expiry-address` configured but there are no queues bound 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] PiotrKlimczak commented on a change in pull
request #3017: ARTEMIS-2649 refactor ORIG message props
Posted by GitBox <gi...@apache.org>.
PiotrKlimczak commented on a change in pull request #3017: ARTEMIS-2649 refactor ORIG message props
URL: https://github.com/apache/activemq-artemis/pull/3017#discussion_r396075152
##########
File path: artemis-server/src/main/java/org/apache/activemq/artemis/core/server/impl/QueueImpl.java
##########
@@ -3418,7 +3419,12 @@ private boolean sendToDeadLetterAddress(final Transaction tx,
ref.acknowledge(tx, AckReason.KILLED, null);
} else {
ActiveMQServerLogger.LOGGER.messageExceededMaxDeliverySendtoDLA(ref, deadLetterAddress, name);
- move(tx, deadLetterAddress, null, ref, false, AckReason.KILLED, null);
+ RoutingStatus status = move(tx, deadLetterAddress, null, ref, false, AckReason.KILLED, null);
+
+ // this shouldn't happen, but in case it does it's better to log a message than just drop the message silently
+ if (status.equals(RoutingStatus.NO_BINDINGS) && server.getAddressSettingsRepository().getMatch(getAddress().toString()).isAutoCreateDeadLetterResources()) {
+ ActiveMQServerLogger.LOGGER.noMatchingBindingsOnDLAWithAutoCreateDLAResources(deadLetterAddress, ref.toString());
Review comment:
Just to be sure we are on the same page.
My only concern is that with fully correct config, Artemis can still lose a message.
We both agreed we can't see such scenario (bug) for now, however potential is there- hence your warning.
Or putting the question the other way round, why do you think we need this warning at all, considering the user has configured auto-create-dead-letter-resources? And I think the answer is- because we both know there is such potential and I am saying we could be more defensive in such scenario.
And I don't care about situation when user misconfigures something, as I fully agree they need to test their config.
The other question should be then:
Is there a scenario where auto-create-dead-letter-resources is configured and the user doesn't want some of the messages to land in any auto-created DLQ- like can we specify additional filter, which will be applied to auto-crated DLQs or this is not possible/planned?
If ability to configure additional filters on auto-created DLQs is there (I am not aware of it by code analysis), then "defensive" approach is indeed in conflict with this feature.
That would be technically the only thing which would justify not having defensive code.
@jbertram 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] PiotrKlimczak commented on a change in pull
request #3017: ARTEMIS-2649 refactor ORIG message props
Posted by GitBox <gi...@apache.org>.
PiotrKlimczak commented on a change in pull request #3017: ARTEMIS-2649 refactor ORIG message props
URL: https://github.com/apache/activemq-artemis/pull/3017#discussion_r401678967
##########
File path: artemis-server/src/main/java/org/apache/activemq/artemis/core/server/impl/QueueImpl.java
##########
@@ -3418,7 +3419,12 @@ private boolean sendToDeadLetterAddress(final Transaction tx,
ref.acknowledge(tx, AckReason.KILLED, null);
} else {
ActiveMQServerLogger.LOGGER.messageExceededMaxDeliverySendtoDLA(ref, deadLetterAddress, name);
- move(tx, deadLetterAddress, null, ref, false, AckReason.KILLED, null);
+ RoutingStatus status = move(tx, deadLetterAddress, null, ref, false, AckReason.KILLED, null);
+
+ // this shouldn't happen, but in case it does it's better to log a message than just drop the message silently
+ if (status.equals(RoutingStatus.NO_BINDINGS) && server.getAddressSettingsRepository().getMatch(getAddress().toString()).isAutoCreateDeadLetterResources()) {
+ ActiveMQServerLogger.LOGGER.noMatchingBindingsOnDLAWithAutoCreateDLAResources(deadLetterAddress, ref.toString());
Review comment:
@jbertram thanks for the answer.
In general I accept the fact that different companies/projects has different practices/approaches etc and there is no single good standard fitting all of them as it all really depends on many factors.
Definitely it is better to have this fix as it is now, rather than not having it at all and keep discussing it for next year or so.
So I am happy with above as long as this is according to Artemis project standards, which I suppose other Artemis commiters should approve.
@jbertram thanks again for your time on this.
----------------------------------------------------------------
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 #3017: ARTEMIS-2649 refactor ORIG message props
Posted by GitBox <gi...@apache.org>.
michaelandrepearce commented on a change in pull request #3017: ARTEMIS-2649 refactor ORIG message props
URL: https://github.com/apache/activemq-artemis/pull/3017#discussion_r395981399
##########
File path: artemis-core-client/src/main/java/org/apache/activemq/artemis/api/core/Message.java
##########
@@ -463,30 +463,40 @@ default void reencode() {
}
default void referenceOriginalMessage(final Message original, String originalQueue) {
- String queueOnMessage = original.getAnnotationString(Message.HDR_ORIGINAL_QUEUE);
+ String originalQueueOnMessage = original.getAnnotationString(Message.HDR_ORIGINAL_QUEUE);
- if (queueOnMessage != null) {
- setAnnotation(Message.HDR_ORIGINAL_QUEUE, queueOnMessage);
- } else if (originalQueue != null) {
- setAnnotation(Message.HDR_ORIGINAL_QUEUE, originalQueue);
+ if (originalQueueOnMessage != null) {
+ setBreadcrumb(Message.HDR_ORIGINAL_QUEUE, originalQueueOnMessage);
}
+ setAnnotation(Message.HDR_ORIGINAL_QUEUE, originalQueue);
+
Object originalID = original.getAnnotation(Message.HDR_ORIG_MESSAGE_ID);
if (originalID != null) {
Review comment:
Messages are moved between queues its only needed to track queues
----------------------------------------------------------------
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] jbertram commented on a change in pull request
#3017: ARTEMIS-2649 refactor ORIG message props
Posted by GitBox <gi...@apache.org>.
jbertram commented on a change in pull request #3017: ARTEMIS-2649 refactor ORIG message props
URL: https://github.com/apache/activemq-artemis/pull/3017#discussion_r396023241
##########
File path: artemis-core-client/src/main/java/org/apache/activemq/artemis/api/core/Message.java
##########
@@ -463,30 +463,40 @@ default void reencode() {
}
default void referenceOriginalMessage(final Message original, String originalQueue) {
- String queueOnMessage = original.getAnnotationString(Message.HDR_ORIGINAL_QUEUE);
+ String originalQueueOnMessage = original.getAnnotationString(Message.HDR_ORIGINAL_QUEUE);
- if (queueOnMessage != null) {
- setAnnotation(Message.HDR_ORIGINAL_QUEUE, queueOnMessage);
- } else if (originalQueue != null) {
- setAnnotation(Message.HDR_ORIGINAL_QUEUE, originalQueue);
+ if (originalQueueOnMessage != null) {
+ setBreadcrumb(Message.HDR_ORIGINAL_QUEUE, originalQueueOnMessage);
}
+ setAnnotation(Message.HDR_ORIGINAL_QUEUE, originalQueue);
+
Object originalID = original.getAnnotation(Message.HDR_ORIG_MESSAGE_ID);
if (originalID != null) {
Review comment:
It seems theoretically possible, but I can't currently conceive of an actual use-case where this would happen. If you can write a test that demonstrates this happening then I'll change 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] PiotrKlimczak commented on issue #3017:
ARTEMIS-2649 refactor ORIG message props
Posted by GitBox <gi...@apache.org>.
PiotrKlimczak commented on issue #3017: ARTEMIS-2649 refactor ORIG message props
URL: https://github.com/apache/activemq-artemis/pull/3017#issuecomment-602014196
@jbertram Thanks for this fix.
I like the implementation as it will allow additionally to see full history of how the message has been diverted.
And indeed with this implementation, the previous problem is not gonna happen, unless someone does something really silly in config.
----------------------------------------------------------------
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] PiotrKlimczak commented on a change in pull
request #3017: ARTEMIS-2649 refactor ORIG message props
Posted by GitBox <gi...@apache.org>.
PiotrKlimczak commented on a change in pull request #3017: ARTEMIS-2649 refactor ORIG message props
URL: https://github.com/apache/activemq-artemis/pull/3017#discussion_r395979710
##########
File path: artemis-server/src/main/java/org/apache/activemq/artemis/core/server/impl/QueueImpl.java
##########
@@ -3418,7 +3419,12 @@ private boolean sendToDeadLetterAddress(final Transaction tx,
ref.acknowledge(tx, AckReason.KILLED, null);
} else {
ActiveMQServerLogger.LOGGER.messageExceededMaxDeliverySendtoDLA(ref, deadLetterAddress, name);
- move(tx, deadLetterAddress, null, ref, false, AckReason.KILLED, null);
+ RoutingStatus status = move(tx, deadLetterAddress, null, ref, false, AckReason.KILLED, null);
+
+ // this shouldn't happen, but in case it does it's better to log a message than just drop the message silently
+ if (status.equals(RoutingStatus.NO_BINDINGS) && server.getAddressSettingsRepository().getMatch(getAddress().toString()).isAutoCreateDeadLetterResources()) {
+ ActiveMQServerLogger.LOGGER.noMatchingBindingsOnDLAWithAutoCreateDLAResources(deadLetterAddress, ref.toString());
Review comment:
The other option would be to move the message to some silly named queue here, like: lostMessages or whatever.
@michaelandrepearce, @clebertsuconic any chance for you to voice your opinion, on whether we are ok to lose a message with warning? As per comment- nobody can see such scenario for now, but potential is there.
----------------------------------------------------------------
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