[
https://issues.apache.org/jira/browse/ARTEMIS-5142?focusedWorklogId=943377&page=com.atlassian.jira.plugin.system.issuetabpanels:worklog-tabpanel#worklog-943377
]
ASF GitHub Bot logged work on ARTEMIS-5142:
-------------------------------------------
Author: ASF GitHub Bot
Created on: 12/Nov/24 16:08
Start Date: 12/Nov/24 16:08
Worklog Time Spent: 10m
Work Description: 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.
Issue Time Tracking
-------------------
Worklog Id: (was: 943377)
Time Spent: 1h 50m (was: 1h 40m)
> Setting 0 for min/max expiry-delivery not working as expected
> -------------------------------------------------------------
>
> Key: ARTEMIS-5142
> URL: https://issues.apache.org/jira/browse/ARTEMIS-5142
> Project: ActiveMQ Artemis
> Issue Type: Improvement
> Reporter: Justin Bertram
> Assignee: Justin Bertram
> Priority: Major
> Labels: pull-request-available
> Time Spent: 1h 50m
> Remaining Estimate: 0h
>
> If {{max-expiry-delay}} or {{min-expiry-delay}} is set to {{0}} messages are
> expired _immediately_ rather than not expired at all as expected.
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
For further information, visit: https://activemq.apache.org/contact
