ARTEMIS-5142 setting 0 for min/max expiry-delivery not working as expected #5334
Conversation
...jdbc-store/src/main/java/org/apache/activemq/artemis/jdbc/store/file/JDBCSequentialFile.java
Outdated
Show resolved
Hide resolved
jbertram
force-pushed
the
ARTEMIS-5142
branch
from
299337e
to
af99bb4
Compare
|
The min and max settings aren't listed in the address-settings doc, might be good to add them with this. (They are covered in the expiry section referenced from the address setting doc for the other expiry setting). |
jbertram
force-pushed
the
ARTEMIS-5142
branch
from
af99bb4
to
81ad29f
Compare
|
@gemmellr, I took care of the documentation. |
jbertram
force-pushed
the
ARTEMIS-5142
branch
from
81ad29f
to
a02194b
Compare
| import org.junit.Test; | ||
| import org.mockito.Mockito; | ||
|
|
||
| public class ExpiryTest { |
There was a problem hiding this comment.
Would be more obvious/discoverable later for this class to be called PostOfficeImplTest
|
|
||
| import org.apache.activemq.artemis.api.core.Message; | ||
| import org.apache.activemq.artemis.core.settings.impl.AddressSettings; | ||
| import org.junit.Test; |
There was a problem hiding this comment.
This is the JUnit 4 test annotation (still present in this module because the vintage runner is needed for some tests), should be using org.junit.jupiter.api.Test
| public class ExpiryTest { | ||
|
|
||
| @Test | ||
| public void testZeroMaxExpiryDelayWhenExpirationSet() { |
There was a problem hiding this comment.
For properly testing the areas being changed, we should include tests with and without the expiration set originally (these 2 both set it), and probably also with the 'expiry-delay' set instead of the min/max ones.
There was a problem hiding this comment.
I added the ones you mentioned plus a few more.
jbertram
force-pushed
the
ARTEMIS-5142
branch
from
a02194b
to
cb291cc
Compare
jbertram
force-pushed
the
ARTEMIS-5142
branch
from
cb291cc
to
1dd02bb
Compare
…ests with both min and max set
| 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)); | ||
| } |
There was a problem hiding this 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.
| } else if (maxExpiration != AddressSettings.DEFAULT_MAX_EXPIRY_DELAY && message.getExpiration() > (System.currentTimeMillis() + maxExpiration)) { | ||
| message.setExpiration(getExpirationToSet(maxExpiration)); |
There was a problem hiding this 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?
|
In addition to the above comments on potential for mismatch between doc and/or expectations and the actual behaviour...I pushed another commit with some more tests, and tweaks to some of the existing tests, to more strictly verify the resulting expiration value set is in the expected range. See what you think. |
No description provided.