cshannon commented on code in PR #1484:
URL: https://github.com/apache/activemq/pull/1484#discussion_r3914044138
##########
activemq-broker/src/main/java/org/apache/activemq/broker/region/BaseDestination.java:
##########
@@ -311,12 +313,42 @@ 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.
+ // An attached durable subscription never permits gc - its
registration and pending messages
+ // live in the destination's store, which gc destroys. Note offline
durable subscriptions
+ // stay attached only with keepDurableSubsActive=true (the default);
brokers running with
+ // keepDurableSubsActive=false forfeit this protection while the
subscriber is offline.
+ return canGcNetwork || (isGcWithOnlyWildcardConsumers() &&
subscription.isWildcard()
+ && !subscription.getConsumerInfo().isDurable());
Review Comment:
@mattrpav - So the TLDR is we don't need to do anything and we can skip the
`subscription.getPendingQueueSize() == 0` check after all because any durable
(offline or not) will be tracked and prevent GC.
I did some testing and I realized our assumption was wrong, the destination
consumer count metric **will track any** durable subscription that exists,
regardless of if it is active or if the keepDurableSubsActive is true or false.
The metric for consumers is **only** decremented on subscription
[deletion](https://github.com/apache/activemq/blob/c1bb9d6613b7713449e588056cf6bfbbfa92988c/activemq-broker/src/main/java/org/apache/activemq/broker/region/Topic.java#L228).
This means that we don't need to change anything or add a check for
subscription.getPendingQueueSize() or durables because:
1. For queues, the message count metric is checked and prevents GC if there
are messages, plus there is consumer count > 0 check to prevent GC if there are
consumers.
2. For topic subscriptions, the consumers > 0 check prevents GC.
3. For online durable subscriptions, the consumers > 0 check prevents GC.
4. For offline durable subscriptions, regardless of whether
keepDurableSubsActive is true or false, those subscriptions are tracked in the
consumer metrics so **destinationStatistics.getConsumers().getCount()> 0** and
prevent GC (regardless of pending messages)
So it's safe to just keep things the way it is.
--
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
