cshannon commented on code in PR #2590:
URL: https://github.com/apache/activemq/pull/2590#discussion_r4195657303
##########
activemq-broker/src/main/java/org/apache/activemq/broker/region/Queue.java:
##########
@@ -2000,8 +1998,20 @@ public void messageExpired(ConnectionContext context,
Subscription subs, Message
} finally {
messagesLock.writeLock().unlock();
}
+ // Advertise and count the expiry only after the store removal and
the
+ // messages decrement (dropMessage) have succeeded. A failed
attempt
+ // signals and counts nothing, so a retried expiry is advertised
and
+ // counted exactly once no matter how many attempts it took.
+ broker.messageExpired(context, reference, subs);
+ destinationStatistics.getExpired().increment();
} catch (IOException e) {
- LOG.error("Failed to remove expired Message from the store ", e);
+ // The expiry CAS (canProcessAsExpired) was consumed by the caller
and
+ // dropMessage has not run: without a reset the failed expiry would
+ // never be retried and the messages counter would stay inflated
while
+ // the row remains orphaned in the store. Reset so a later expiry
+ // check can process this message again.
+ reference.getMessage().resetProcessAsExpired();
Review Comment:
The main thing I am wondering here is if we should always reset for any
IOException. It kind of depends on where it failed. If it fails at
broker.messageExpired(context, reference, subs); then I don't think we want to
re-run again as that is primarily just for advisory support. I think we only
want to reset if the removeMessage() line fails right? In reality it probably
doesn't matter too much as I wouldn't expect the IOException to be thrown
except during the removeMessage() call but maybe we should do something like:
```java
try {
removeMessage(context, subs, (QueueMessageReference) reference);
} catch (IOException e) {
reference.getMessage().resetProcessAsExpired();
throw e;
}
```
##########
activemq-broker/src/main/java/org/apache/activemq/broker/region/Queue.java:
##########
@@ -2000,8 +1998,20 @@ public void messageExpired(ConnectionContext context,
Subscription subs, Message
} finally {
messagesLock.writeLock().unlock();
}
+ // Advertise and count the expiry only after the store removal and
the
Review Comment:
I think you can cut down on the verboseness of these comments. Adding
comments is good but it's actually harder to read the code when there is so
much text. You could just say something like
```
// Process advisory and update metrics only after expiration succeeds
```
--
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
