You are viewing a plain text version of this content. The canonical link for it is here.
Posted to gitbox@activemq.apache.org by "mattrpav (via GitHub)" <gi...@apache.org> on 2026/09/21 16:10:39 UTC
[PR] [#2583] Temporary expiration failure can lead to incorrect queue metrics accounting [activemq]
mattrpav opened a new pull request, #2590:
URL: https://github.com/apache/activemq/pull/2590
An IOException when attempting an expiration, incorrectly leaves the message count but registers the expiredCount.
This change does two things:
1. Add a natural retry to message expiration if it fails on IOException
2. Count the expiration and fire the advisory after the expiration-to-store is successful
--
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-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For additional commands, e-mail: gitbox-help@activemq.apache.org
For further information, visit: https://activemq.apache.org/contact
Re: [PR] [#2583] Temporary expiration failure can lead to incorrect queue metrics accounting [activemq]
Posted by "cshannon (via GitHub)" <gi...@apache.org>.
cshannon commented on PR #2590:
URL: https://github.com/apache/activemq/pull/2590#issuecomment-5779041969
And yes I understand the failover case being temporary, but we can't just solve that case. This needs to also consider non failover when IOExceptions are fatal.
--
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-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For additional commands, e-mail: gitbox-help@activemq.apache.org
For further information, visit: https://activemq.apache.org/contact
Re: [PR] [#2583] Temporary expiration failure can lead to incorrect queue metrics accounting [activemq]
Posted by "cshannon (via GitHub)" <gi...@apache.org>.
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: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For additional commands, e-mail: gitbox-help@activemq.apache.org
For further information, visit: https://activemq.apache.org/contact
Re: [PR] [#2583] Temporary expiration failure can lead to incorrect queue metrics accounting [activemq]
Posted by "mattrpav (via GitHub)" <gi...@apache.org>.
mattrpav commented on PR #2590:
URL: https://github.com/apache/activemq/pull/2590#issuecomment-5779470566
@cshannon the accounting is incorrect. If the write fails, the queue size and expiry count are incorrect when this occurs. This change addresses the accounting by moving the expiredCount to after the write is successful, and by allowing for retrying of the expiry, the queue size metric won't be incorrect -- queueSize > 0 when there are no messages to be processed.
The IOException is already being caught today. The new log may be better phrased to say 'expiry _may_ be retried'.
IOExceptionHandler decision to move broker into inactive mode is orthogonal.
--
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-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For additional commands, e-mail: gitbox-help@activemq.apache.org
For further information, visit: https://activemq.apache.org/contact
Re: [PR] [#2583] Temporary expiration failure can lead to incorrect queue metrics accounting [activemq]
Posted by "cshannon (via GitHub)" <gi...@apache.org>.
cshannon commented on PR #2590:
URL: https://github.com/apache/activemq/pull/2590#issuecomment-5779013864
I don't really understand why we are retrying on IOException. That's usually a pretty fatal exception, retrying seems bad and I could see it getting into a loop.
What possible IOException could happen that is retriable? The broker will just shut down on IOException in many cases. So having a blanket retry doesn't seem like a great idea.
--
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-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For additional commands, e-mail: gitbox-help@activemq.apache.org
For further information, visit: https://activemq.apache.org/contact