gemmellr commented on code in PR #5327:
URL: https://github.com/apache/activemq-artemis/pull/5327#discussion_r1872954781
##########
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:
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?
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
For further information, visit: https://activemq.apache.org/contact
