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/13 18:44:28 UTC
[GitHub] [activemq-artemis] clebertsuconic opened a new pull request, #4029: ARTEMIS-3778 Streamline Expiration Reaping
clebertsuconic opened a new pull request, #4029:
URL: https://github.com/apache/activemq-artemis/pull/4029
Instead of holding a thread and an iterator, we should instead keep moving to next references
without holding any threads. Just with callbacks.
--
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
[GitHub] [activemq-artemis] clebertsuconic commented on a diff in pull request #4029: ARTEMIS-3778 Streamline Expiration Reaping
Posted by GitBox <gi...@apache.org>.
clebertsuconic commented on code in PR #4029:
URL: https://github.com/apache/activemq-artemis/pull/4029#discussion_r850409627
##########
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:
the very first version of this was using the executor of the queue.... which I later switched to use the same executor on the done method, so it does not need to be volatile..
it's left over.. I forgot to remove it... thanks
--
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
[GitHub] [activemq-artemis] clebertsuconic merged pull request #4029: ARTEMIS-3778 Streamline Expiration Reaping
Posted by GitBox <gi...@apache.org>.
clebertsuconic merged PR #4029:
URL: https://github.com/apache/activemq-artemis/pull/4029
--
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
[GitHub] [activemq-artemis] clebertsuconic commented on a diff in pull request #4029: ARTEMIS-3778 Streamline Expiration Reaping
Posted by GitBox <gi...@apache.org>.
clebertsuconic commented on code in PR #4029:
URL: https://github.com/apache/activemq-artemis/pull/4029#discussion_r850415283
##########
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:
removed the left over 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
[GitHub] [activemq-artemis] gemmellr commented on a diff in pull request #4029: ARTEMIS-3778 Streamline Expiration Reaping
Posted by GitBox <gi...@apache.org>.
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
[GitHub] [activemq-artemis] gemmellr commented on a diff in pull request #4029: ARTEMIS-3778 Streamline Expiration Reaping
Posted by GitBox <gi...@apache.org>.
gemmellr commented on code in PR #4029:
URL: https://github.com/apache/activemq-artemis/pull/4029#discussion_r850461538
##########
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:
I wouldnt say this comment still applies. It seems strange to log that it is "done" in the cases it has just been logged that it isnt finished.
--
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
[GitHub] [activemq-artemis] clebertsuconic commented on a diff in pull request #4029: ARTEMIS-3778 Streamline Expiration Reaping
Posted by GitBox <gi...@apache.org>.
clebertsuconic commented on code in PR #4029:
URL: https://github.com/apache/activemq-artemis/pull/4029#discussion_r850410780
##########
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:
the iterator is always used on the same thread.
--
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
[GitHub] [activemq-artemis] jbertram commented on a diff in pull request #4029: ARTEMIS-3778 Streamline Expiration Reaping
Posted by GitBox <gi...@apache.org>.
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);
}
```
--
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
[GitHub] [activemq-artemis] clebertsuconic commented on a diff in pull request #4029: ARTEMIS-3778 Streamline Expiration Reaping
Posted by GitBox <gi...@apache.org>.
clebertsuconic commented on code in PR #4029:
URL: https://github.com/apache/activemq-artemis/pull/4029#discussion_r850580759
##########
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:
done
--
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
[GitHub] [activemq-artemis] clebertsuconic commented on a diff in pull request #4029: ARTEMIS-3778 Streamline Expiration Reaping
Posted by GitBox <gi...@apache.org>.
clebertsuconic commented on code in PR #4029:
URL: https://github.com/apache/activemq-artemis/pull/4029#discussion_r850575993
##########
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:
I added the currentQueue just for debugging. but I guess it could have a double usage now...
--
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
[GitHub] [activemq-artemis] clebertsuconic commented on a diff in pull request #4029: ARTEMIS-3778 Streamline Expiration Reaping
Posted by GitBox <gi...@apache.org>.
clebertsuconic commented on code in PR #4029:
URL: https://github.com/apache/activemq-artemis/pull/4029#discussion_r850579917
##########
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:
@jbertram good idea.. it does look better.. thanks
--
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
[GitHub] [activemq-artemis] gemmellr commented on pull request #4029: ARTEMIS-3778 Streamline Expiration Reaping
Posted by GitBox <gi...@apache.org>.
gemmellr commented on PR #4029:
URL: https://github.com/apache/activemq-artemis/pull/4029#issuecomment-1099235202
LGTM
--
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