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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
