[GitHub] [activemq-artemis] jbertram commented on a diff in pull request #4029: ARTEMIS-3778 Streamline Expiration Reaping

[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