[
https://issues.apache.org/jira/browse/ARTEMIS-3778?focusedWorklogId=756990&page=com.atlassian.jira.plugin.system.issuetabpanels:worklog-tabpanel#worklog-756990
]
ASF GitHub Bot logged work on ARTEMIS-3778:
-------------------------------------------
Author: ASF GitHub Bot
Created on: 14/Apr/22 14:57
Start Date: 14/Apr/22 14:57
Worklog Time Spent: 10m
Work Description: jbertram commented on code in PR #4029:
URL: https://github.com/apache/activemq-artemis/pull/4029#discussion_r850531662
##########
artemis-server/src/main/java/org/apache/activemq/artemis/core/postoffice/impl/PostOfficeImpl.java:
##########
@@ -1846,43 +1844,45 @@ private final class ExpiryReaper extends
ActiveMQScheduledComponent {
super(scheduledExecutorService, executor, checkPeriod, timeUnit,
onDemand);
}
- volatile CountDownLatch inUseLatch;
+ private Iterator<Queue> iterator;
+ // this is for logger.debug only
+ private Queue currentQueue;
@Override
- public void stop() {
- super.stop();
- // this will do a best effort to stop the current latch.
- // no big deal if it failed. this is just to optimize this component
stop.
- CountDownLatch latch = inUseLatch;
- if (latch != null) {
- latch.countDown();
+ public void run() {
+ if (iterator != null) {
+ logger.debugf("A previous reaping call has not finished yet, and
it is currently working on %s", currentQueue);
+ return;
}
+
+ iterator = iterableOf(getLocalQueues()).iterator();
+
+ moveNext();
}
+ private void done() {
+ executor.execute(this::moveNext);
+ }
- @Override
- public void run() {
- // The reaper thread should be finished case the PostOffice is gone
- // This is to avoid leaks on PostOffice between stops and starts
- for (Queue queue : iterableOf(getLocalQueues())) {
- if (!isStarted()) {
- break;
- }
- try {
- CountDownLatch latch = new CountDownLatch(1);
- this.inUseLatch = latch;
- queue.expireReferences(latch::countDown);
- // the idea is in fact to block the Reaper while the Queue is
executing reaping.
- // This would avoid another eventual expiry to be called if the
period for reaping is too small
- // This should also avoid bursts in CPU consumption because of
the expiry reaping
- if (!latch.await(10, TimeUnit.SECONDS)) {
- ActiveMQServerLogger.LOGGER.errorExpiringMessages(new
TimeoutException(queue.getName().toString()));
- }
- } catch (Exception e) {
- ActiveMQServerLogger.LOGGER.errorExpiringMessages(e);
- }
+ private void moveNext() {
+ Queue queue;
+ if (!iterator.hasNext() || !this.isStarted()) {
+ queue = null;
+ } else {
+ queue = iterator.next();
}
+
+ if (queue == null) {
Review Comment:
This is probably a nit-pick, but it seems to me this could be refactored a
bit to be more concise, e.g.:
```java
private void moveNext() {
if (!iterator.hasNext() || !this.isStarted()) {
iterator = null;
currentQueue = null;
return;
}
currentQueue = iterator.next();
// we will expire messages on this queue, once done we move to the
next queue
currentQueue.expireReferences(this::done);
}
```
Issue Time Tracking
-------------------
Worklog Id: (was: 756990)
Time Spent: 1h 20m (was: 1h 10m)
> Reaper reporting TimeoutException on Reaper threads
> ---------------------------------------------------
>
> Key: ARTEMIS-3778
> URL: https://issues.apache.org/jira/browse/ARTEMIS-3778
> Project: ActiveMQ Artemis
> Issue Type: Bug
> Components: Broker
> Affects Versions: 2.21.0
> Reporter: Clebert Suconic
> Priority: Major
> Fix For: 2.22.0
>
> Time Spent: 1h 20m
> Remaining Estimate: 0h
>
> If a queue is holding too many references, and it takes more than 10 seconds
> to iterate on references, the reaper will log an error:
> AMQ224013: failed to expire messages for queue:
> java.util.concurrent.TimeoutException
> Instead I should streamline reaping to resume the next queue when one queue
> is done. This way no thread is held blocking and we just streamline the
> process.
--
This message was sent by Atlassian Jira
(v8.20.1#820001)
