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?

You updated the behaviour a bit, bit it still differs from before in the sense that for the combined expire method it only runs the refCountForConsumers.check(); if there is a broker message plugin, rather than always doing it before for this old expire method (the other old expire method only did it if there is a plugin). Was it wrong to always check it in this method before, i.e was the other method correct not to do it without a plugin? Essentially, why should it need a broker message plugin to do it?

clebertsuconic · 2024-12-04T16:45:41Z

@gemmellr I added a commit to be squashed with a change...

gemmellr · 2024-12-06T10:00:39Z

Left a new comment in one of the previous comment 'threads' #5327 (comment)

…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

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

clebertsuconic force-pushed the ARTEMIS-5119 branch 2 times, most recently from 5a9600d to 2a35892 Compare December 4, 2024 16:36

clebertsuconic marked this pull request as draft December 4, 2024 16:45

clebertsuconic added 2 commits December 18, 2024 21:00

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

fb5e346

…l Expiry Queue

Addressing comments from Robbie trying to equalize the methods

d073f53

clebertsuconic force-pushed the ARTEMIS-5119 branch from 2a35892 to d073f53 Compare December 19, 2024 02:00

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

gemmellr Dec 6, 2024

clebertsuconic commented Dec 4, 2024

gemmellr commented Dec 6, 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

Choose a reason for hiding this comment

clebertsuconic commented Dec 4, 2024

gemmellr commented Dec 6, 2024