You are viewing a plain text version of this content. The canonical link for it is here.
Posted to gitbox@activemq.apache.org by "jbertram (via GitHub)" <gi...@apache.org> on 2024/11/01 16:19:57 UTC
[PR] ARTEMIS-5142 setting 0 for min/max expiry-delivery not working as expected [activemq-artemis]
jbertram opened a new pull request, #5334:
URL: https://github.com/apache/activemq-artemis/pull/5334
(no comment)
--
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-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For additional commands, e-mail: gitbox-help@activemq.apache.org
For further information, visit: https://activemq.apache.org/contact
Re: [PR] ARTEMIS-5142 setting 0 for min/max expiry-delivery not working as expected [activemq-artemis]
Posted by "gemmellr (via GitHub)" <gi...@apache.org>.
gemmellr commented on PR #5334:
URL: https://github.com/apache/activemq-artemis/pull/5334#issuecomment-2500663688
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?
--
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-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For additional commands, e-mail: gitbox-help@activemq.apache.org
For further information, visit: https://activemq.apache.org/contact
Re: [PR] ARTEMIS-5142 setting 0 for min/max expiry-delivery not working as expected [activemq-artemis]
Posted by "gemmellr (via GitHub)" <gi...@apache.org>.
gemmellr commented on PR #5334:
URL: https://github.com/apache/activemq-artemis/pull/5334#issuecomment-2494104152
Other than those bits I commented on where its clearly not ok? :)
--
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-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For additional commands, e-mail: gitbox-help@activemq.apache.org
For further information, visit: https://activemq.apache.org/contact
Re: [PR] ARTEMIS-5142 setting 0 for min/max expiry-delivery not working as expected [activemq-artemis]
Posted by "clebertsuconic (via GitHub)" <gi...@apache.org>.
clebertsuconic commented on PR #5334:
URL: https://github.com/apache/activemq-artemis/pull/5334#issuecomment-2494120395
@gemmellr I said.. loTm... not clearly to you :)
--
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-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For additional commands, e-mail: gitbox-help@activemq.apache.org
For further information, visit: https://activemq.apache.org/contact
Re: [PR] ARTEMIS-5142 setting 0 for min/max expiry-delivery not working as expected [activemq-artemis]
Posted by "gemmellr (via GitHub)" <gi...@apache.org>.
gemmellr commented on PR #5334:
URL: https://github.com/apache/activemq-artemis/pull/5334#issuecomment-2470947324
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.
--
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-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For additional commands, e-mail: gitbox-help@activemq.apache.org
For further information, visit: https://activemq.apache.org/contact
Re: [PR] ARTEMIS-5142 setting 0 for min/max expiry-delivery not working as expected [activemq-artemis]
Posted by "gemmellr (via GitHub)" <gi...@apache.org>.
gemmellr commented on PR #5334:
URL: https://github.com/apache/activemq-artemis/pull/5334#issuecomment-2494265494
Feel free to squash them
--
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-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For additional commands, e-mail: gitbox-help@activemq.apache.org
For further information, visit: https://activemq.apache.org/contact
Re: [PR] ARTEMIS-5142 setting 0 for min/max expiry-delivery not working as expected [activemq-artemis]
Posted by "jbertram (via GitHub)" <gi...@apache.org>.
jbertram commented on code in PR #5334:
URL: https://github.com/apache/activemq-artemis/pull/5334#discussion_r1878709166
##########
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:
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.
--
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-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For additional commands, e-mail: gitbox-help@activemq.apache.org
For further information, visit: https://activemq.apache.org/contact
Re: [PR] ARTEMIS-5142 setting 0 for min/max expiry-delivery not working as expected [activemq-artemis]
Posted by "gemmellr (via GitHub)" <gi...@apache.org>.
gemmellr commented on code in PR #5334:
URL: https://github.com/apache/activemq-artemis/pull/5334#discussion_r1856877053
##########
artemis-server/src/test/java/org/apache/activemq/artemis/core/postoffice/impl/PostOfficeImplTest.java:
##########
@@ -0,0 +1,252 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.activemq.artemis.core.postoffice.impl;
+
+import org.apache.activemq.artemis.api.core.Message;
+import org.apache.activemq.artemis.core.settings.impl.AddressSettings;
+import org.junit.jupiter.api.Test;
+import org.mockito.ArgumentCaptor;
+import org.mockito.Mockito;
+
+import static org.junit.jupiter.api.Assertions.assertNotNull;
+import static org.junit.jupiter.api.Assertions.assertTrue;
+
+public class PostOfficeImplTest {
+
+ private static final int EXPIRATION_DELTA = 5000;
+
+ @Test
+ public void testZeroMaxExpiryDelayWhenExpirationNotSet() {
+ Message mockMessage = Mockito.mock(Message.class);
+ Mockito.when(mockMessage.getExpiration()).thenReturn(0L);
+ PostOfficeImpl.applyExpiryDelay(mockMessage, new AddressSettings().setMaxExpiryDelay(0L));
+ Mockito.verify(mockMessage).setExpiration(Mockito.anyLong());
Review Comment:
Another consequence of removing the 'dont set on 0' bit was this test (and testZeroMinExpiryDelayWhenExpirationNotSet further down) is now almost useless. Beforehand the test checked _nothing_ was set (because the value was already as expected), which was a very tight check of the correct value then being in effect. Thats why I didnt originally need to update it along with the other tests to more closely check that an appropriate value being set, as it was already as precise as possible. After the update it is now instead just allowing _any_ value to be set, which is effectively no check at all that the correct value is in effect. At the very least it should check 0 is set if thats whats expected to happen (though as earlier comment, I think you were correct to avoid that and it shouldnt)
--
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-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For additional commands, e-mail: gitbox-help@activemq.apache.org
For further information, visit: https://activemq.apache.org/contact
Re: [PR] ARTEMIS-5142 setting 0 for min/max expiry-delivery not working as expected [activemq-artemis]
Posted by "gemmellr (via GitHub)" <gi...@apache.org>.
gemmellr commented on code in PR #5334:
URL: https://github.com/apache/activemq-artemis/pull/5334#discussion_r1826099819
##########
pom.xml:
##########
Review Comment:
These changes seem unrelated
--
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-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For additional commands, e-mail: gitbox-help@activemq.apache.org
For further information, visit: https://activemq.apache.org/contact
Re: [PR] ARTEMIS-5142 setting 0 for min/max expiry-delivery not working as expected [activemq-artemis]
Posted by "jbertram (via GitHub)" <gi...@apache.org>.
jbertram commented on code in PR #5334:
URL: https://github.com/apache/activemq-artemis/pull/5334#discussion_r1857060527
##########
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:
Done.
--
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-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For additional commands, e-mail: gitbox-help@activemq.apache.org
For further information, visit: https://activemq.apache.org/contact
Re: [PR] ARTEMIS-5142 setting 0 for min/max expiry-delivery not working as expected [activemq-artemis]
Posted by "jbertram (via GitHub)" <gi...@apache.org>.
jbertram commented on code in PR #5334:
URL: https://github.com/apache/activemq-artemis/pull/5334#discussion_r1837162409
##########
artemis-server/src/test/java/org/apache/activemq/artemis/core/postoffice/impl/ExpiryTest.java:
##########
@@ -0,0 +1,41 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.activemq.artemis.core.postoffice.impl;
+
+import org.apache.activemq.artemis.api.core.Message;
+import org.apache.activemq.artemis.core.settings.impl.AddressSettings;
+import org.junit.Test;
+import org.mockito.Mockito;
+
+public class ExpiryTest {
Review Comment:
Renamed.
##########
artemis-server/src/test/java/org/apache/activemq/artemis/core/postoffice/impl/ExpiryTest.java:
##########
@@ -0,0 +1,41 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.activemq.artemis.core.postoffice.impl;
+
+import org.apache.activemq.artemis.api.core.Message;
+import org.apache.activemq.artemis.core.settings.impl.AddressSettings;
+import org.junit.Test;
Review Comment:
Fixed.
--
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-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For additional commands, e-mail: gitbox-help@activemq.apache.org
For further information, visit: https://activemq.apache.org/contact
Re: [PR] ARTEMIS-5142 setting 0 for min/max expiry-delivery not working as expected [activemq-artemis]
Posted by "jbertram (via GitHub)" <gi...@apache.org>.
jbertram commented on PR #5334:
URL: https://github.com/apache/activemq-artemis/pull/5334#issuecomment-2494291839
I decided to keep your commit. Merge when ready.
--
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-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For additional commands, e-mail: gitbox-help@activemq.apache.org
For further information, visit: https://activemq.apache.org/contact
Re: [PR] ARTEMIS-5142 setting 0 for min/max expiry-delivery not working as expected [activemq-artemis]
Posted by "clebertsuconic (via GitHub)" <gi...@apache.org>.
clebertsuconic commented on PR #5334:
URL: https://github.com/apache/activemq-artemis/pull/5334#issuecomment-2494046693
this LGTM as long as tests are working fine
--
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-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For additional commands, e-mail: gitbox-help@activemq.apache.org
For further information, visit: https://activemq.apache.org/contact
Re: [PR] ARTEMIS-5142 setting 0 for min/max expiry-delivery not working as expected [activemq-artemis]
Posted by "gemmellr (via GitHub)" <gi...@apache.org>.
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-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For additional commands, e-mail: gitbox-help@activemq.apache.org
For further information, visit: https://activemq.apache.org/contact
Re: [PR] ARTEMIS-5142 setting 0 for min/max expiry-delivery not working as expected [activemq-artemis]
Posted by "gemmellr (via GitHub)" <gi...@apache.org>.
gemmellr closed pull request #5334: ARTEMIS-5142 setting 0 for min/max expiry-delivery not working as expected
URL: https://github.com/apache/activemq-artemis/pull/5334
--
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-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For additional commands, e-mail: gitbox-help@activemq.apache.org
For further information, visit: https://activemq.apache.org/contact
Re: [PR] ARTEMIS-5142 setting 0 for min/max expiry-delivery not working as expected [activemq-artemis]
Posted by "jbertram (via GitHub)" <gi...@apache.org>.
jbertram commented on code in PR #5334:
URL: https://github.com/apache/activemq-artemis/pull/5334#discussion_r1854200869
##########
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 idea with `&& maxExpiration != 0` was simply to avoid setting the expiration to `0` when it already was `0`. However, I see the issue with the logic that you mention so I'm taking it out. Good catch!
--
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-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For additional commands, e-mail: gitbox-help@activemq.apache.org
For further information, visit: https://activemq.apache.org/contact
Re: [PR] ARTEMIS-5142 setting 0 for min/max expiry-delivery not working as expected [activemq-artemis]
Posted by "jbertram (via GitHub)" <gi...@apache.org>.
jbertram commented on PR #5334:
URL: https://github.com/apache/activemq-artemis/pull/5334#issuecomment-2494155919
@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?
--
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-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For additional commands, e-mail: gitbox-help@activemq.apache.org
For further information, visit: https://activemq.apache.org/contact
Re: [PR] ARTEMIS-5142 setting 0 for min/max expiry-delivery not working as expected [activemq-artemis]
Posted by "gemmellr (via GitHub)" <gi...@apache.org>.
gemmellr commented on code in PR #5334:
URL: https://github.com/apache/activemq-artemis/pull/5334#discussion_r1838353398
##########
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 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.
--
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-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For additional commands, e-mail: gitbox-help@activemq.apache.org
For further information, visit: https://activemq.apache.org/contact
Re: [PR] ARTEMIS-5142 setting 0 for min/max expiry-delivery not working as expected [activemq-artemis]
Posted by "jbertram (via GitHub)" <gi...@apache.org>.
jbertram commented on code in PR #5334:
URL: https://github.com/apache/activemq-artemis/pull/5334#discussion_r1826284603
##########
pom.xml:
##########
Review Comment:
Fixed.
--
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-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For additional commands, e-mail: gitbox-help@activemq.apache.org
For further information, visit: https://activemq.apache.org/contact
Re: [PR] ARTEMIS-5142 setting 0 for min/max expiry-delivery not working as expected [activemq-artemis]
Posted by "gemmellr (via GitHub)" <gi...@apache.org>.
gemmellr commented on PR #5334:
URL: https://github.com/apache/activemq-artemis/pull/5334#issuecomment-2452296690
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).
--
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-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For additional commands, e-mail: gitbox-help@activemq.apache.org
For further information, visit: https://activemq.apache.org/contact
Re: [PR] ARTEMIS-5142 setting 0 for min/max expiry-delivery not working as expected [activemq-artemis]
Posted by "jbertram (via GitHub)" <gi...@apache.org>.
jbertram commented on code in PR #5334:
URL: https://github.com/apache/activemq-artemis/pull/5334#discussion_r1878619808
##########
artemis-server/src/test/java/org/apache/activemq/artemis/core/postoffice/impl/PostOfficeImplTest.java:
##########
@@ -99,6 +99,14 @@ public void testZeroMinExpiryDelayWhenExpirationSet() {
Mockito.verify(mockMessage).setExpiration(0L);
}
+ @Test
+ public void testZeroMinExpiryDelayWhenExpirationSetInFuture() {
+ Message mockMessage = Mockito.mock(Message.class);
+ Mockito.when(mockMessage.getExpiration()).thenReturn(System.currentTimeMillis() + 500_000L);
+ PostOfficeImpl.applyExpiryDelay(mockMessage, new AddressSettings().setMinExpiryDelay(0L));
+ Mockito.verify(mockMessage).setExpiration(0L);
+ }
Review Comment:
Thinking about this more...I'm not sure a `min-expiry-delay` of `0` even makes sense. The overall goal of this change is to enable users to never expire incoming messages regardless of the message's existing expiration. If they want to do that then they should set `max-expiry-delay` to `0` and leave `expiry-delay` and `min-expiry-delay` unset (i.e. `-1`).
Setting `min-expiry-delay` to `0` is really just meaningless. A message will not realistically have an expiration below `0` and if there's no max set (as in this test) then anything above `0` should remain unchanged.
I'm going to clarify the Jira to make the use-case explicit.
##########
artemis-server/src/test/java/org/apache/activemq/artemis/core/postoffice/impl/PostOfficeImplTest.java:
##########
@@ -99,6 +99,14 @@ public void testZeroMinExpiryDelayWhenExpirationSet() {
Mockito.verify(mockMessage).setExpiration(0L);
}
+ @Test
+ public void testZeroMinExpiryDelayWhenExpirationSetInFuture() {
+ Message mockMessage = Mockito.mock(Message.class);
+ Mockito.when(mockMessage.getExpiration()).thenReturn(System.currentTimeMillis() + 500_000L);
+ PostOfficeImpl.applyExpiryDelay(mockMessage, new AddressSettings().setMinExpiryDelay(0L));
+ Mockito.verify(mockMessage).setExpiration(0L);
+ }
Review Comment:
Thinking about this more...I'm not sure a `min-expiry-delay` of `0` even makes sense. The overall goal of this change is to enable users to never expire incoming messages regardless of the message's existing expiration. If they want to do that then they should set `max-expiry-delay` to `0` and leave `expiry-delay` and `min-expiry-delay` unset (i.e. `-1`).
Setting `min-expiry-delay` to `0` is really just meaningless. A message will not realistically have an expiration below `0` and if there's no max set (as in this test) then anything above `0` should remain unchanged.
I'm going to clarify the Jira to make the targeted use-case explicit.
--
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-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For additional commands, e-mail: gitbox-help@activemq.apache.org
For further information, visit: https://activemq.apache.org/contact
Re: [PR] ARTEMIS-5142 setting 0 for min/max expiry-delivery not working as expected [activemq-artemis]
Posted by "jbertram (via GitHub)" <gi...@apache.org>.
jbertram commented on code in PR #5334:
URL: https://github.com/apache/activemq-artemis/pull/5334#discussion_r1854212230
##########
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:
Fixing!
--
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-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For additional commands, e-mail: gitbox-help@activemq.apache.org
For further information, visit: https://activemq.apache.org/contact
Re: [PR] ARTEMIS-5142 setting 0 for min/max expiry-delivery not working as expected [activemq-artemis]
Posted by "jbertram (via GitHub)" <gi...@apache.org>.
jbertram commented on code in PR #5334:
URL: https://github.com/apache/activemq-artemis/pull/5334#discussion_r1857061404
##########
artemis-server/src/test/java/org/apache/activemq/artemis/core/postoffice/impl/PostOfficeImplTest.java:
##########
@@ -0,0 +1,252 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.activemq.artemis.core.postoffice.impl;
+
+import org.apache.activemq.artemis.api.core.Message;
+import org.apache.activemq.artemis.core.settings.impl.AddressSettings;
+import org.junit.jupiter.api.Test;
+import org.mockito.ArgumentCaptor;
+import org.mockito.Mockito;
+
+import static org.junit.jupiter.api.Assertions.assertNotNull;
+import static org.junit.jupiter.api.Assertions.assertTrue;
+
+public class PostOfficeImplTest {
+
+ private static final int EXPIRATION_DELTA = 5000;
+
+ @Test
+ public void testZeroMaxExpiryDelayWhenExpirationNotSet() {
+ Message mockMessage = Mockito.mock(Message.class);
+ Mockito.when(mockMessage.getExpiration()).thenReturn(0L);
+ PostOfficeImpl.applyExpiryDelay(mockMessage, new AddressSettings().setMaxExpiryDelay(0L));
+ Mockito.verify(mockMessage).setExpiration(Mockito.anyLong());
Review Comment:
Fixed.
--
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-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For additional commands, e-mail: gitbox-help@activemq.apache.org
For further information, visit: https://activemq.apache.org/contact
Re: [PR] ARTEMIS-5142 setting 0 for min/max expiry-delivery not working as expected [activemq-artemis]
Posted by "jbertram (via GitHub)" <gi...@apache.org>.
jbertram commented on PR #5334:
URL: https://github.com/apache/activemq-artemis/pull/5334#issuecomment-2532278865
> ...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.
--
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-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For additional commands, e-mail: gitbox-help@activemq.apache.org
For further information, visit: https://activemq.apache.org/contact
Re: [PR] ARTEMIS-5142 setting 0 for min/max expiry-delivery not working as expected [activemq-artemis]
Posted by "jbertram (via GitHub)" <gi...@apache.org>.
jbertram commented on code in PR #5334:
URL: https://github.com/apache/activemq-artemis/pull/5334#discussion_r1878994734
##########
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 put together #5390 which does this very thing. Let me know what you think.
--
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-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For additional commands, e-mail: gitbox-help@activemq.apache.org
For further information, visit: https://activemq.apache.org/contact
Re: [PR] ARTEMIS-5142 setting 0 for min/max expiry-delivery not working as expected [activemq-artemis]
Posted by "gemmellr (via GitHub)" <gi...@apache.org>.
gemmellr commented on code in PR #5334:
URL: https://github.com/apache/activemq-artemis/pull/5334#discussion_r1856756767
##########
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:
I didnt think it should be removed, just otherwise fixed.
Allowing it to call setExpiration(0) registers the message as having been changed, when it has not been. I think you were correct to avoid that case occurring.
--
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-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For additional commands, e-mail: gitbox-help@activemq.apache.org
For further information, visit: https://activemq.apache.org/contact
Re: [PR] ARTEMIS-5142 setting 0 for min/max expiry-delivery not working as expected [activemq-artemis]
Posted by "jbertram (via GitHub)" <gi...@apache.org>.
jbertram commented on code in PR #5334:
URL: https://github.com/apache/activemq-artemis/pull/5334#discussion_r1826055003
##########
artemis-jdbc-store/src/main/java/org/apache/activemq/artemis/jdbc/store/file/JDBCSequentialFile.java:
##########
@@ -92,6 +92,7 @@ public class JDBCSequentialFile implements SequentialFile {
this.filename = filename;
this.extension = filename.contains(".") ? filename.substring(filename.lastIndexOf(".") + 1, filename.length()) : "";
this.executor = executor;
+ new Exception(driver.toString()).printStackTrace();
Review Comment:
Indeed!
--
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-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For additional commands, e-mail: gitbox-help@activemq.apache.org
For further information, visit: https://activemq.apache.org/contact
Re: [PR] ARTEMIS-5142 setting 0 for min/max expiry-delivery not working as expected [activemq-artemis]
Posted by "jbertram (via GitHub)" <gi...@apache.org>.
jbertram commented on PR #5334:
URL: https://github.com/apache/activemq-artemis/pull/5334#issuecomment-2452570714
@gemmellr, I took care of the documentation.
--
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-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For additional commands, e-mail: gitbox-help@activemq.apache.org
For further information, visit: https://activemq.apache.org/contact
Re: [PR] ARTEMIS-5142 setting 0 for min/max expiry-delivery not working as expected [activemq-artemis]
Posted by "gemmellr (via GitHub)" <gi...@apache.org>.
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-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For additional commands, e-mail: gitbox-help@activemq.apache.org
For further information, visit: https://activemq.apache.org/contact
Re: [PR] ARTEMIS-5142 setting 0 for min/max expiry-delivery not working as expected [activemq-artemis]
Posted by "gemmellr (via GitHub)" <gi...@apache.org>.
gemmellr commented on PR #5334:
URL: https://github.com/apache/activemq-artemis/pull/5334#issuecomment-2551665136
Closing in favour of #5390
--
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-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For additional commands, e-mail: gitbox-help@activemq.apache.org
For further information, visit: https://activemq.apache.org/contact
Re: [PR] ARTEMIS-5142 setting 0 for min/max expiry-delivery not working as expected [activemq-artemis]
Posted by "gemmellr (via GitHub)" <gi...@apache.org>.
gemmellr commented on code in PR #5334:
URL: https://github.com/apache/activemq-artemis/pull/5334#discussion_r1856756767
##########
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:
I didnt think it should be removed, just otherwise fixed.
Allowing it to call setExpirartion(0) registers the message as having been changed, when it has not been. I think you were correct to avoid that case occurring.
--
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-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For additional commands, e-mail: gitbox-help@activemq.apache.org
For further information, visit: https://activemq.apache.org/contact
Re: [PR] ARTEMIS-5142 setting 0 for min/max expiry-delivery not working as expected [activemq-artemis]
Posted by "tabish121 (via GitHub)" <gi...@apache.org>.
tabish121 commented on code in PR #5334:
URL: https://github.com/apache/activemq-artemis/pull/5334#discussion_r1826049376
##########
artemis-jdbc-store/src/main/java/org/apache/activemq/artemis/jdbc/store/file/JDBCSequentialFile.java:
##########
@@ -92,6 +92,7 @@ public class JDBCSequentialFile implements SequentialFile {
this.filename = filename;
this.extension = filename.contains(".") ? filename.substring(filename.lastIndexOf(".") + 1, filename.length()) : "";
this.executor = executor;
+ new Exception(driver.toString()).printStackTrace();
Review Comment:
This seems like it was not supposed to make it into the PR ?
--
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-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For additional commands, e-mail: gitbox-help@activemq.apache.org
For further information, visit: https://activemq.apache.org/contact
Re: [PR] ARTEMIS-5142 setting 0 for min/max expiry-delivery not working as expected [activemq-artemis]
Posted by "jbertram (via GitHub)" <gi...@apache.org>.
jbertram commented on code in PR #5334:
URL: https://github.com/apache/activemq-artemis/pull/5334#discussion_r1878619808
##########
artemis-server/src/test/java/org/apache/activemq/artemis/core/postoffice/impl/PostOfficeImplTest.java:
##########
@@ -99,6 +99,14 @@ public void testZeroMinExpiryDelayWhenExpirationSet() {
Mockito.verify(mockMessage).setExpiration(0L);
}
+ @Test
+ public void testZeroMinExpiryDelayWhenExpirationSetInFuture() {
+ Message mockMessage = Mockito.mock(Message.class);
+ Mockito.when(mockMessage.getExpiration()).thenReturn(System.currentTimeMillis() + 500_000L);
+ PostOfficeImpl.applyExpiryDelay(mockMessage, new AddressSettings().setMinExpiryDelay(0L));
+ Mockito.verify(mockMessage).setExpiration(0L);
+ }
Review Comment:
Thinking about this more...I'm not sure a `min-expiry-delay` of `0` even makes sense. The overall goal of this change is to enable users to never expire incoming messages regardless of the message's existing expiration. If they want to do that then they should set `max-expiry-delay` to `0` and leave `expiry-delay` and `min-expiry-delay` unset (i.e. `-1`).
Setting `min-expiry-delay` to `0` is really just meaningless. A message will not realistically have an expiration below `0` and if there's no max set (as in this test) then anything above `0` should remain unchanged.
I'm going to clarify the Jira to make the targeted use-case explicit.
--
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-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For additional commands, e-mail: gitbox-help@activemq.apache.org
For further information, visit: https://activemq.apache.org/contact
Re: [PR] ARTEMIS-5142 setting 0 for min/max expiry-delivery not working as expected [activemq-artemis]
Posted by "jbertram (via GitHub)" <gi...@apache.org>.
jbertram commented on code in PR #5334:
URL: https://github.com/apache/activemq-artemis/pull/5334#discussion_r1878619808
##########
artemis-server/src/test/java/org/apache/activemq/artemis/core/postoffice/impl/PostOfficeImplTest.java:
##########
@@ -99,6 +99,14 @@ public void testZeroMinExpiryDelayWhenExpirationSet() {
Mockito.verify(mockMessage).setExpiration(0L);
}
+ @Test
+ public void testZeroMinExpiryDelayWhenExpirationSetInFuture() {
+ Message mockMessage = Mockito.mock(Message.class);
+ Mockito.when(mockMessage.getExpiration()).thenReturn(System.currentTimeMillis() + 500_000L);
+ PostOfficeImpl.applyExpiryDelay(mockMessage, new AddressSettings().setMinExpiryDelay(0L));
+ Mockito.verify(mockMessage).setExpiration(0L);
+ }
Review Comment:
Thinking about this more...I'm not sure a `min-expiry-delay` of `0` even makes sense. The overall goal of this change is to enable users to never expire incoming messages regardless of the message's existing expiration. If they want to do that then they should set `max-expiry-delay` to `0` and leave `expiry-delay` and `min-expiry-delay` unset (i.e. `-1`).
Setting `min-expiry-delay` to `0` is really just meaningless. A message will not realistically have an expiration below `0` and if there's no max set (as in this test) then anything above `0` should remain unchanged.
--
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-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For additional commands, e-mail: gitbox-help@activemq.apache.org
For further information, visit: https://activemq.apache.org/contact
Re: [PR] ARTEMIS-5142 setting 0 for min/max expiry-delivery not working as expected [activemq-artemis]
Posted by "gemmellr (via GitHub)" <gi...@apache.org>.
gemmellr commented on code in PR #5334:
URL: https://github.com/apache/activemq-artemis/pull/5334#discussion_r1830555393
##########
artemis-server/src/test/java/org/apache/activemq/artemis/core/postoffice/impl/ExpiryTest.java:
##########
@@ -0,0 +1,41 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.activemq.artemis.core.postoffice.impl;
+
+import org.apache.activemq.artemis.api.core.Message;
+import org.apache.activemq.artemis.core.settings.impl.AddressSettings;
+import org.junit.Test;
+import org.mockito.Mockito;
+
+public class ExpiryTest {
Review Comment:
Would be more obvious/discoverable later for this class to be called PostOfficeImplTest
##########
artemis-server/src/test/java/org/apache/activemq/artemis/core/postoffice/impl/ExpiryTest.java:
##########
@@ -0,0 +1,41 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.activemq.artemis.core.postoffice.impl;
+
+import org.apache.activemq.artemis.api.core.Message;
+import org.apache.activemq.artemis.core.settings.impl.AddressSettings;
+import org.junit.Test;
Review 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
##########
artemis-server/src/test/java/org/apache/activemq/artemis/core/postoffice/impl/ExpiryTest.java:
##########
@@ -0,0 +1,41 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.activemq.artemis.core.postoffice.impl;
+
+import org.apache.activemq.artemis.api.core.Message;
+import org.apache.activemq.artemis.core.settings.impl.AddressSettings;
+import org.junit.Test;
+import org.mockito.Mockito;
+
+public class ExpiryTest {
+
+ @Test
+ public void testZeroMaxExpiryDelayWhenExpirationSet() {
Review 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.
--
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-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For additional commands, e-mail: gitbox-help@activemq.apache.org
For further information, visit: https://activemq.apache.org/contact
Re: [PR] ARTEMIS-5142 setting 0 for min/max expiry-delivery not working as expected [activemq-artemis]
Posted by "jbertram (via GitHub)" <gi...@apache.org>.
jbertram commented on code in PR #5334:
URL: https://github.com/apache/activemq-artemis/pull/5334#discussion_r1837162767
##########
artemis-server/src/test/java/org/apache/activemq/artemis/core/postoffice/impl/ExpiryTest.java:
##########
@@ -0,0 +1,41 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.activemq.artemis.core.postoffice.impl;
+
+import org.apache.activemq.artemis.api.core.Message;
+import org.apache.activemq.artemis.core.settings.impl.AddressSettings;
+import org.junit.Test;
+import org.mockito.Mockito;
+
+public class ExpiryTest {
+
+ @Test
+ public void testZeroMaxExpiryDelayWhenExpirationSet() {
Review Comment:
I added the ones you mentioned plus a few more.
--
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-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For additional commands, e-mail: gitbox-help@activemq.apache.org
For further information, visit: https://activemq.apache.org/contact