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?
--
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-unsubscr...@activemq.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscr...@activemq.apache.org
For additional commands, e-mail: gitbox-h...@activemq.apache.org
For further information, visit: https://activemq.apache.org/contact
