[
https://issues.apache.org/jira/browse/ARTEMIS-5119?focusedWorklogId=942402&page=com.atlassian.jira.plugin.system.issuetabpanels:worklog-tabpanel#worklog-942402
]
ASF GitHub Bot logged work on ARTEMIS-5119:
-------------------------------------------
Author: ASF GitHub Bot
Created on: 07/Nov/24 11:15
Start Date: 07/Nov/24 11:15
Worklog Time Spent: 10m
Work Description: gemmellr commented on code in PR #5327:
URL: https://github.com/apache/activemq-artemis/pull/5327#discussion_r1832485608
##########
artemis-server/src/main/java/org/apache/activemq/artemis/core/server/impl/QueueImpl.java:
##########
@@ -2129,36 +2130,85 @@ 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();
+ expire(null, ref, consumer, delivering);
+ }
- if (logger.isTraceEnabled()) {
- logger.trace("moving expired reference {} to address = {} from
queue={}", ref, addressSettings.getExpiryAddress(), name);
+ private void expire(final Transaction tx, final MessageReference ref, final
ServerConsumer consumer, boolean delivering) throws Exception {
+ AddressSettings settingsToUse =
getMessageAddressSettings(ref.getMessage());
+ SimpleString expiryAddress = settingsToUse.getExpiryAddress();
+
+ if (logger.isDebugEnabled()) {
+ logger.debug("expire on {}/{}, consumer={}, expiryAddress={}",
this.address, this.name, consumer, expiryAddress);
+ }
+
+ if (expiryAddress != null && expiryAddress.length() != 0) {
+ String messageAddress = ref.getMessage().getAddress();
+
+ if (messageAddress == null) {
+ // in the unlikely event where a message does not have an address
stored on the message itself,
+ // we will get the address from the current queue
+ messageAddress = String.valueOf(getAddress());
}
+ createExpiryResources(messageAddress, settingsToUse);
- move(null, addressSettings.getExpiryAddress(), null, ref, false,
AckReason.EXPIRED, consumer, null, delivering);
+ Bindings bindingList =
postOffice.lookupBindingsForAddress(expiryAddress);
+
+ 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);
+ }
Review Comment:
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?
##########
artemis-server/src/main/java/org/apache/activemq/artemis/core/server/impl/QueueImpl.java:
##########
@@ -2129,36 +2130,85 @@ 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();
+ expire(null, ref, consumer, delivering);
+ }
- if (logger.isTraceEnabled()) {
- logger.trace("moving expired reference {} to address = {} from
queue={}", ref, addressSettings.getExpiryAddress(), name);
+ private void expire(final Transaction tx, final MessageReference ref, final
ServerConsumer consumer, boolean delivering) throws Exception {
+ AddressSettings settingsToUse =
getMessageAddressSettings(ref.getMessage());
+ SimpleString expiryAddress = settingsToUse.getExpiryAddress();
+
+ if (logger.isDebugEnabled()) {
+ logger.debug("expire on {}/{}, consumer={}, expiryAddress={}",
this.address, this.name, consumer, expiryAddress);
+ }
+
+ if (expiryAddress != null && expiryAddress.length() != 0) {
+ String messageAddress = ref.getMessage().getAddress();
+
+ if (messageAddress == null) {
+ // in the unlikely event where a message does not have an address
stored on the message itself,
+ // we will get the address from the current queue
+ messageAddress = String.valueOf(getAddress());
}
+ createExpiryResources(messageAddress, settingsToUse);
- move(null, addressSettings.getExpiryAddress(), null, ref, false,
AckReason.EXPIRED, consumer, null, delivering);
+ Bindings bindingList =
postOffice.lookupBindingsForAddress(expiryAddress);
+
+ 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);
+ }
} else {
- logger.trace("expiry is null, just acking expired message for
reference {} from queue={}", ref, name);
+ if (!printErrorExpiring) {
+ printErrorExpiring = true;
+ // print this only once
+ ActiveMQServerLogger.LOGGER.errorExpiringReferencesNoAddress(name);
+ }
- acknowledge(null, ref, AckReason.EXPIRED, consumer, delivering);
+ acknowledge(tx, ref, AckReason.EXPIRED, consumer, delivering);
}
- // potentially auto-delete this queue if this expired the last message
- refCountForConsumers.check();
Review Comment:
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?
Issue Time Tracking
-------------------
Worklog Id: (was: 942402)
Time Spent: 1h 40m (was: 1.5h)
> Messages that expire in store-and-forward (sf-) queues should go to original
> queue's configured expiry queue
> ------------------------------------------------------------------------------------------------------------
>
> Key: ARTEMIS-5119
> URL: https://issues.apache.org/jira/browse/ARTEMIS-5119
> Project: ActiveMQ Artemis
> Issue Type: Bug
> Affects Versions: 2.37.0
> Reporter: Rakhi Kumari
> Assignee: Clebert Suconic
> Priority: Major
> Labels: pull-request-available
> Fix For: 2.39.0
>
> Time Spent: 1h 40m
> Remaining Estimate: 0h
>
> When messages expire in the store-and-forward queues, they go to the
> store-and-forward queue's expiry queue. It would be better if the messages go
> to the original queue's expiry queue.
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
For further information, visit: https://activemq.apache.org/contact
