gemmellr commented on code in PR #5334:
URL: https://github.com/apache/activemq-artemis/pull/5334#discussion_r1838353398
##########
artemis-server/src/main/java/org/apache/activemq/artemis/core/postoffice/impl/PostOfficeImpl.java:
##########
@@ -1366,17 +1367,32 @@ private static void applyExpiryDelay(Message message,
AddressSettings settings)
message.setExpiration(System.currentTimeMillis() +
expirationOverride);
}
} else {
- long minExpiration = settings.getMinExpiryDelay();
- long maxExpiration = settings.getMaxExpiryDelay();
-
- if (maxExpiration != AddressSettings.DEFAULT_MAX_EXPIRY_DELAY &&
(message.getExpiration() == 0 || message.getExpiration() >
(System.currentTimeMillis() + maxExpiration))) {
- message.setExpiration(System.currentTimeMillis() + maxExpiration);
+ // if the incoming message has NO expiration then apply the max if
set and if not set then apply the min if set
+ if (message.getExpiration() == 0) {
+ if (maxExpiration != AddressSettings.DEFAULT_MAX_EXPIRY_DELAY &&
maxExpiration != 0) {
+ message.setExpiration(getExpirationToSet(maxExpiration));
+ } else if (minExpiration !=
AddressSettings.DEFAULT_MIN_EXPIRY_DELAY && minExpiration != 0) {
+ message.setExpiration(getExpirationToSet(minExpiration));
+ }
Review Comment:
The recent "&& maxExpiration != 0" addition seems like a logic bug. Setting
the max to 0 is still classed as setting it, and the doc says that in this
situation the max value will be applied if set...however this would instead
deliberately _not_ set it and then perhaps apply the _minimum_ if it were set
(which initially seemed like a strange setup, but perhaps not given the next
comment...and either way it doesnt seem like anything prevents it...and its
debatable whether a non-zero minimum is still lower than never-expire maximum).
The maxExpiration != 0 check would need to be in its own nested if to do
what the doc says. Alternatively, the getExpirationToSet(maxExpiration) call
could always be made, and the message.setExpiration call only applied if that
value differed from the existing message expiration.
--
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-unsubscr...@activemq.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscr...@activemq.apache.org
For additional commands, e-mail: gitbox-h...@activemq.apache.org
For further information, visit: https://activemq.apache.org/contact
