gemmellr commented on code in PR #5334:
URL: https://github.com/apache/activemq-artemis/pull/5334#discussion_r1858442725
##########
artemis-server/src/main/java/org/apache/activemq/artemis/core/postoffice/impl/PostOfficeImpl.java:
##########
@@ -1366,17 +1367,36 @@ 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);
- } else if (minExpiration != AddressSettings.DEFAULT_MIN_EXPIRY_DELAY
&& message.getExpiration() < (System.currentTimeMillis() + minExpiration)) {
- message.setExpiration(System.currentTimeMillis() + minExpiration);
+ // 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) {
+ if (maxExpiration != 0) {
+ message.setExpiration(getExpirationToSet(maxExpiration));
+ }
+ } else if (minExpiration !=
AddressSettings.DEFAULT_MIN_EXPIRY_DELAY) {
+ if (minExpiration != 0) {
+ message.setExpiration(getExpirationToSet(minExpiration));
+ }
+ }
+ } else if (maxExpiration != AddressSettings.DEFAULT_MAX_EXPIRY_DELAY
&& message.getExpiration() >= (System.currentTimeMillis() + maxExpiration)) {
+ message.setExpiration(getExpirationToSet(maxExpiration));
+ } else if (minExpiration != AddressSettings.DEFAULT_MIN_EXPIRY_DELAY
&& (minExpiration == 0 || message.getExpiration() < (System.currentTimeMillis()
+ minExpiration))) {
+ message.setExpiration(getExpirationToSet(minExpiration));
Review Comment:
I didnt change the max-expiry-delay=0 handling, as I'm now debating what it
should do overall. It has a similar but reversed behaviour, that it wont do
anything for an expiration in the past, but will for present/future
expirations. Which on one hand seems perfectly fair; originally already
expired, definitely 'shorter expiration' than a maximum of never-expire.
However on the other hand its also totally at odds with it resetting a
future-expiration to instead never-expire.
The special handling for max-expiry-delay=0 meaning dont-expire, mainly
seems to be there / necessary because of the earlier application when a message
has no expiration set meaning it then gets max-expiry-delay applied if
configured (which previously then immediately expired before these changes, and
now would remain with no-expiry).
In the later cases here of a message which does have an expiration already,
it seems a foggier that it should remove it for max-expiry-delay. For such
messages, max-expiry-delay is documented to only affect existing expirations
greater than the maximum, reducing them to the maximum. A max-expiry-delay of 0
meaning no-expiration, is then arguably still a 'larger value' (never/infinite)
than any specific expiration point that might be set, arguably meaning it
shouldnt be changed due to the max-expiry-delay. Then, if you do want to stop
such a message expiring which already have an expiration set, the way is
instead setting a min-expiry-delay=0 to handle that case. If on the other hand
a setting of 0 just always means 'set no-expiration' even for the
max-expiry-delay like it does in most cases now, then should it not also do
that even for the messages that originally had expiration in the past? (by
adding a similar maxExpiration=0 escape to the check as I did for minExpiration)
##########
docs/user-manual/message-expiry.adoc:
##########
@@ -67,8 +67,8 @@ If `expiry-delay` is _not set_ then minimum and maximum
expiry delay values can
Semantics are as follows:
* Messages _without_ an expiration will be set to `max-expiry-delay`.
-If `max-expiry-delay` is not defined then the message will be set to
`min-expiry-delay`.
-If `min-expiry-delay` is not defined then the message will not be changed.
+** If `max-expiry-delay` is not defined then the message will be set to
`min-expiry-delay`.
+** If `min-expiry-delay` is not defined then the message will not be changed.
Review Comment:
Whatever we end up doing, the specific 0=special handling that applies
should probably be documented, its not mentioned anywhere currently
--
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
