ARTEMIS-5142 setting 0 for min/max expiry-delivery not working as expected

[jbertram]

No description provided.

[gemmellr]

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]

@gemmellr, I took care of the documentation.

[gemmellr]

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.

[clebertsuconic]

this LGTM as long as tests are working fine

[gemmellr]

Other than those bits I commented on where its clearly not ok? :)

[clebertsuconic]

@gemmellr I said.. loTm... not clearly to you :)

[jbertram]

@gemmellr, the extra/modified tests in your commit look good. How do you want to merge them? Do you want to rebase on the push -f I'm about to do or do you want me to just incorporate them into my own?

[gemmellr]

Feel free to squash them

[jbertram]

I decided to keep your commit. Merge when ready.

[gemmellr]

I was about to merge this earlier when I took another look at the overall method and changes, and noticed what I would consider a bug, and a couple other potential bugs or things to [re]consider.

I instead rebased the existing changes and added another commit (temporary, just to make it easy to see the new changes) fixing the main issue I noticed around min-expiry-delay = 0, where it wouldnt do anything for a messsage with expiration actually set to the future, with it instead only updating expirations already from the past (like the existing test was using by setting it to 1). That seemed wrong, as configuring a minimum of '0 means dont expire' should remove any expiration, regardless whether it started as past/present/future, if a minimum of 0 is being treated as a special case for no expiration to occur.

[gemmellr]

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)

[jbertram]

Hmmm...So maybe the current behavior of min-expiry-delay and max-expiry-delay is actually correct in a strict semantic interpretation and instead of making 0 behave in a special way we should just add a new address-setting to strip the expiration from incoming messages so they never expire categorically which is the semantic that the Jira is really aimed at.

[jbertram]

I put together #5390 which does this very thing. Let me know what you think.

[gemmellr]

Whatever we end up doing, the specific 0=special handling that applies should probably be documented, its not mentioned anywhere currently

[gemmellr]

Also, I noticed the regular expiry-delay (not min/max) setting has the same '0 makes it set expire now' behaviour because it will set the current time as the expiration. It is subtly different from the min/max case in that the setting only ever applies to messages with an existing expiration, so setting expiry-delay to 0 really makes no sense to begin with, so it could more reasonably just be left as it is, but I wonder if it should also special-case it like min/max now do?

[jbertram]

...setting expiry-delay to 0 really makes no sense to begin with...

That surely means someone will do it. I'll add a test and update the logic to deal with this.