mattrpav commented on code in PR #1484:
URL: https://github.com/apache/activemq/pull/1484#discussion_r3899400456


##########
activemq-broker/src/main/java/org/apache/activemq/broker/region/BaseDestination.java:
##########
@@ -311,12 +313,46 @@ public final MessageStore getMessageStore() {
 
     @Override
     public boolean isActive() {
-        boolean isActive = destinationStatistics.getConsumers().getCount() > 0 
||
-                           destinationStatistics.getProducers().getCount() > 0;
-        if (isActive && isGcWithNetworkConsumers() && 
destinationStatistics.getConsumers().getCount() > 0) {
-            isActive = hasRegularConsumers(getConsumers());
+        // if we have producers then we are active
+        if (destinationStatistics.getProducers().getCount() > 0) {
+            return true;
         }
-        return isActive;
+
+        // Check if we have active consumers that should prevent GC
+        if (destinationStatistics.getConsumers().getCount() > 0) {
+            // if we have consumers and both gcWithNetwork and gcOnlyWildcard 
consumers
+            // are false we can just return true, otherwise we need to check 
each consumer
+            return (!isGcWithNetworkConsumers() && 
!isGcWithOnlyWildcardConsumers()) ||
+                    hasActiveConsumers();
+        }
+
+        return false;
+    }
+
+    protected Predicate<Subscription> canGcConsumer = subscription -> {
+        // if isGcWithNetworkConsumers() is true and this is a network 
subscription then we can GC
+        boolean canGcNetwork = isGcWithNetworkConsumers() && 
subscription.getConsumerInfo().isNetworkSubscription();
+        // if isGcWithOnlyWildcardConsumers() is true and this is a 
non-durable wildcard then we can GC.
+        // Durable subscriptions never permit gc - their registration and 
pending messages live in the
+        // destination's store, which gc destroys, so collecting would break 
the durability guarantee.
+        return canGcNetwork || (isGcWithOnlyWildcardConsumers() && 
subscription.isWildcard()

Review Comment:
   1. I have an updated unit test that validates this.
   2. I think supported durable subscriptions would be expensive. We'd have to 
check each durable subscription's size on each topic to ensure it is empty 
every sweep. 
   
   Options:
   
   a. Keep this change non-durable-topics and queues-only (preserves pre-change 
behavior)
   b. Bite off durable topic subscription support. 
   
   



-- 
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]


---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
For further information, visit: https://activemq.apache.org/contact


Reply via email to