ARTEMIS-5119 Expired Messages on Cluster SNF should to to the original Expiry Queue #5327

clebertsuconic · 2024-10-30T16:50:18Z

No description provided.

artemis-server/src/main/java/org/apache/activemq/artemis/core/server/impl/QueueImpl.java

...t/java/org/apache/activemq/artemis/tests/integration/cluster/expiry/ClusteredExpiryTest.java

...rc/test/java/org/apache/activemq/artemis/tests/integration/server/ExpireQueueSuffixTest.java

artemis-server/src/main/java/org/apache/activemq/artemis/core/server/impl/QueueImpl.java

gemmellr · 2024-11-01T15:49:42Z

artemis-server/src/main/java/org/apache/activemq/artemis/core/server/impl/QueueImpl.java

@@ -2129,14 +2130,26 @@ public void expire(final MessageReference ref) throws Exception {
    *           hence no information about delivering statistics should be updated. */
   @Override
   public void expire(final MessageReference ref, final ServerConsumer consumer, boolean delivering) throws Exception {
-      if (addressSettings.getExpiryAddress() != null) {
-         createExpiryResources();
+      AddressSettings settingsToUse = getMessageAddressSettings(ref.getMessage());


The expireFromScan method goes on to do "SimpleString expiryAddress = settingsToUse.getExpiryAddress();" and then use that variable in [most of] the places the value is used. Its strange for two very similar related methods right next to each other to be differing in that. Would be more readable later to either both use the variable, or neither use the variable.

I will merge these two methods.. I was afraid of touching something unrelated.. but it's the best choice given how it's progressed.

Yeah I wasn't suggesting to combine them as they did seem sufficiently different on the backend to justify the two methods. Its just the initial bits were very similar so it would have been nice for them to be set out the same way for readability.

Combining them seems to have rearranged at least the order of some stuff for one method, or added some stuff around bindings that other method didnt do before...I'd need to take a much fuller look to see what I think of those changes, but I need to finish up just now.

The only difference I could see is the use of transactions or not. the use of consumers or not...

those could be set as null and they would have the same semantics... I think it should be good.

Testsuite is 100% passing.

@gemmellr since you mentioned you are looking to review this.. I will leave this PR open until next week.. no rush about merging this yet.. although it would be nice before next release.. but it's not happening next week anyways... so I will leave it open for now.

...t/java/org/apache/activemq/artemis/tests/integration/cluster/expiry/ClusteredExpiryTest.java

artemis-server/src/main/java/org/apache/activemq/artemis/core/server/impl/QueueImpl.java

gemmellr · 2024-11-01T18:00:23Z

artemis-server/src/main/java/org/apache/activemq/artemis/core/server/impl/QueueImpl.java

@@ -2129,14 +2130,26 @@ public void expire(final MessageReference ref) throws Exception {
    *           hence no information about delivering statistics should be updated. */
   @Override
   public void expire(final MessageReference ref, final ServerConsumer consumer, boolean delivering) throws Exception {
-      if (addressSettings.getExpiryAddress() != null) {
-         createExpiryResources();
+      AddressSettings settingsToUse = getMessageAddressSettings(ref.getMessage());


Yeah I wasn't suggesting to combine them as they did seem sufficiently different on the backend to justify the two methods. Its just the initial bits were very similar so it would have been nice for them to be set out the same way for readability.

Combining them seems to have rearranged at least the order of some stuff for one method, or added some stuff around bindings that other method didnt do before...I'd need to take a much fuller look to see what I think of those changes, but I need to finish up just now.

gemmellr · 2024-11-07T11:03:38Z

artemis-server/src/main/java/org/apache/activemq/artemis/core/server/impl/QueueImpl.java

+         if (bindingList == null || bindingList.getBindings().isEmpty()) {
+            ActiveMQServerLogger.LOGGER.errorExpiringReferencesNoBindings(expiryAddress);
+            acknowledge(tx, ref, AckReason.EXPIRED, null, delivering);
+         } else {
+            move(tx, expiryAddress, null, ref, false, AckReason.EXPIRED, consumer, null, delivering);
+         }


Until the most recent commit updates, before you combined the existing 'expire' method with your then-added new 'expireFromScan' method, the existing 'expire' did not do this check and would simply try the move. (Or if there was no expireAddress defined at all, it would log once that there wasnt any.) This seems like it changes things by introducing a potential per-message warning log message about the bindings. Was that desired, given you didn't add it originally when the methods were seperate?

gemmellr · 2024-11-07T11:14:34Z

artemis-server/src/main/java/org/apache/activemq/artemis/core/server/impl/QueueImpl.java

-      // potentially auto-delete this queue if this expired the last message
-      refCountForConsumers.check();


The existing 'expire' method with consumer always did this, before checking the broker plugins. It still did this when you had added the 'expireWithScan' method. Now that you combined the methods, this is only done if there is a tx, and only after checking the broker plugins (and only if there is a message plugin, even though this is a queue-level check). These both seem strange differences from just consolidating some methods. Was one of them wrong before?

…l Expiry Queue

clebertsuconic force-pushed the ARTEMIS-5119 branch 2 times, most recently from 47e931a to a3df301 Compare October 30, 2024 17:17

gemmellr reviewed Nov 1, 2024

View reviewed changes

clebertsuconic force-pushed the ARTEMIS-5119 branch 7 times, most recently from e9adf6f to 71cc83a Compare November 1, 2024 15:35

gemmellr reviewed Nov 1, 2024

View reviewed changes

clebertsuconic force-pushed the ARTEMIS-5119 branch 3 times, most recently from dbd5345 to b7a3b7f Compare November 1, 2024 16:56

gemmellr reviewed Nov 1, 2024

View reviewed changes

clebertsuconic force-pushed the ARTEMIS-5119 branch 2 times, most recently from 1ede4cf to 21e436f Compare November 1, 2024 18:54

gemmellr reviewed Nov 7, 2024

View reviewed changes

ARTEMIS-5119 Expired Messages on Cluster SNF should to to the origina…

85c9f81

…l Expiry Queue

clebertsuconic force-pushed the ARTEMIS-5119 branch from 21e436f to 85c9f81 Compare November 21, 2024 15:55

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ARTEMIS-5119 Expired Messages on Cluster SNF should to to the original Expiry Queue #5327

ARTEMIS-5119 Expired Messages on Cluster SNF should to to the original Expiry Queue #5327

clebertsuconic commented Oct 30, 2024

gemmellr Nov 1, 2024

clebertsuconic Nov 1, 2024

gemmellr Nov 1, 2024

clebertsuconic Nov 1, 2024

clebertsuconic Nov 1, 2024

gemmellr Nov 1, 2024

gemmellr Nov 7, 2024

gemmellr Nov 7, 2024

		// potentially auto-delete this queue if this expired the last message
		refCountForConsumers.check();

ARTEMIS-5119 Expired Messages on Cluster SNF should to to the original Expiry Queue #5327

Are you sure you want to change the base?

ARTEMIS-5119 Expired Messages on Cluster SNF should to to the original Expiry Queue #5327

Conversation

clebertsuconic commented Oct 30, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment