You are viewing a plain text version of this content. The canonical link for it is here.
Posted to gitbox@activemq.apache.org by GitBox <gi...@apache.org> on 2022/04/14 11:49:01 UTC
[GitHub] [activemq-artemis] gemmellr commented on a diff in pull request #4029: ARTEMIS-3778 Streamline Expiration Reaping
gemmellr commented on code in PR #4029:
URL: https://github.com/apache/activemq-artemis/pull/4029#discussion_r850314227
##########
artemis-server/src/main/java/org/apache/activemq/artemis/core/postoffice/impl/PostOfficeImpl.java:
##########
@@ -1846,43 +1844,47 @@ private final class ExpiryReaper extends ActiveMQScheduledComponent {
super(scheduledExecutorService, executor, checkPeriod, timeUnit, onDemand);
}
- volatile CountDownLatch inUseLatch;
+ volatile Iterator<Queue> iterator;
Review Comment:
Can it be private? Does it actually need to be volatile? If so that suggests multiple threads setting/using it, in which case the rest of the usage probably isnt safe (see later comment).
##########
artemis-server/src/main/java/org/apache/activemq/artemis/core/server/impl/QueueImpl.java:
##########
@@ -2434,13 +2430,13 @@ public void run() {
}
if (++elementsIterated >= MAX_DELIVERIES_IN_LOOP) {
logger.debug("Breaking loop of expiring");
- scannerRunning.incrementAndGet();
+ rescheduled = true;
Review Comment:
Relating to the line above this one, rather than this line...
Rather than just logging _"Breaking loop of expiring"_ it might be nice to note the queue name as the completion message does, and mention it is rescheduling.
##########
artemis-server/src/main/java/org/apache/activemq/artemis/core/postoffice/impl/PostOfficeImpl.java:
##########
@@ -1846,43 +1844,47 @@ private final class ExpiryReaper extends ActiveMQScheduledComponent {
super(scheduledExecutorService, executor, checkPeriod, timeUnit, onDemand);
}
- volatile CountDownLatch inUseLatch;
+ volatile Iterator<Queue> iterator;
@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();
- }
Review Comment:
There is a 2 line comment left above this, which appears to be about this code, so it should be removed too.
In fact the whole method can seemingly be removed, all it does now is call the super.
EDIT: or, could/should stop() be clearing the iterator as well rather than just relying on 'moveNext()' to do so later?
##########
artemis-server/src/main/java/org/apache/activemq/artemis/core/server/impl/QueueImpl.java:
##########
@@ -2454,10 +2450,10 @@ public void run() {
if (doneCallback != null) {
doneCallback.run();
- doneCallback = null;
}
}
+
Review Comment:
Empty line addition seems unecessary...but helpful to refer to the existing log message below it.
The _"Scanning for expires on " + QueueImpl.this.getName() + " done"_ message doesnt line up with when it calls the _doneCallback_, as it would be logged every time through, even after the rescheduling. It might be nice if the 'done' messaging did align as it would then work in concert with the other [suggested] logging to be much clearer on what is happening when.
##########
artemis-server/src/main/java/org/apache/activemq/artemis/core/postoffice/impl/PostOfficeImpl.java:
##########
@@ -1846,43 +1844,47 @@ private final class ExpiryReaper extends ActiveMQScheduledComponent {
super(scheduledExecutorService, executor, checkPeriod, timeUnit, onDemand);
}
- volatile CountDownLatch inUseLatch;
+ volatile Iterator<Queue> iterator;
@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();
- }
}
-
@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);
- }
+ if (iterator != null) {
+ logger.debug("iterator is not finished yet");
Review Comment:
The message could make it more obvious what isnt finished to avoid needing to check the source, e.g "Existing expiry reaper iterator is not finished yet, not beginning new sweep."
It would also be good to then add related messages, such as add below that it _is_ starting a new expiry sweep. Perhaps even add another one later that it has finished a sweep, in moveNext(). That way you can easily see when it starts/finishes/skips.
##########
artemis-server/src/main/java/org/apache/activemq/artemis/core/postoffice/impl/PostOfficeImpl.java:
##########
@@ -1846,43 +1844,47 @@ private final class ExpiryReaper extends ActiveMQScheduledComponent {
super(scheduledExecutorService, executor, checkPeriod, timeUnit, onDemand);
}
- volatile CountDownLatch inUseLatch;
+ volatile Iterator<Queue> iterator;
@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();
- }
}
-
@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);
- }
+ if (iterator != null) {
+ logger.debug("iterator is not finished yet");
+ return;
}
+
+ iterator = iterableOf(getLocalQueues()).iterator();
+
+ moveNext();
+ }
+
+ private void done() {
+ executor.execute(this::moveNext);
+ }
+
+ private void moveNext() {
+ Queue queue;
+ if (!iterator.hasNext() || !this.isStarted()) {
+ queue = null;
+ } else {
+ queue = iterator.next();
+ }
+
+ if (queue == null) {
+ iterator = null;
+ return;
+ }
Review Comment:
Related to earlier comments...if _iterator_ needs to be volatile due to multiple threads setting it, it would probably be needed to assign it to a new variable and use that (after a null check) to ensure it is consistently used for e.g hasNext() and later next(), and not being be re-read on each use and potentially changing or being nulled between calls (or even before getting into this method).
--
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