cshannon commented on code in PR #1484:
URL: https://github.com/apache/activemq/pull/1484#discussion_r3896275722
##########
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:
Thinking about this more I think we just need to do some more
testing/investigation for durables and the wildcard case because as you noted
durables get created on real destinations when it's a wildcard. (including if
the wildcard destination itself exists). maybe some improved tests would help
clarify.
I think we just want to make sure of 2 things:
1. If the new flag is false (the default) the existing behavior with cleanup
shouldn't change. That includes whether the sub is a wildcard or not and
whether keepDurableSubsActive is true or false, etc.
2. if the new flag is set to cleanup when wildcard, we just want to make
sure it makes sense for durables too. I'm not sure we need to do anything
special for durables because if keepDurableSubsActive = true of course will
just keep the destinations active as they count as a consumer. For the case
where keepDurableSubsActive = false, a durable gets registered on the specific
topic (even if a wildcard initially). I'm not sure we want to block GC in that
case just f it's offline and no messages it seems like it might be ok to
remove...but only if the current behavior today also would remove it in that
case (needs testing)
--
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
