gemmellr commented on code in PR #5334:
URL: https://github.com/apache/activemq-artemis/pull/5334#discussion_r1838367944
##########
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));
+ }
+ } else if (maxExpiration != AddressSettings.DEFAULT_MAX_EXPIRY_DELAY
&& message.getExpiration() > (System.currentTimeMillis() + maxExpiration)) {
+ message.setExpiration(getExpirationToSet(maxExpiration));
Review Comment:
So this and the segment below seems to try handling the 'max/min expiration
delay 0' special case, setting the expiration to 0. One covers \>current and
the other covers \<current.
However if setting both settings to 0, presumably for an implied
do-no-expiry-ever, then this setup will currently miss the case where the value
exactly matches the current time reference, which will then be left unchanged
and be allowed to expire which could be considered against the expectations for
this special case?
##########
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 would admittedly be a strange setup, but 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
