cshannon commented on code in PR #1484:
URL: https://github.com/apache/activemq/pull/1484#discussion_r3914360103
##########
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 -
I did some testing and I realized my 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).
So going back and forth I think what you have is probably best. My original
pushback was not wanting to treat wildcards as special but I agree there isn't
any other good mechanism to prevent data loss for durables. The current code
only considers network subs, so it just won't delete a destination if there are
durables otherwise. Now, we could delete if there are durables but the consumer
is a wildcard so that is probably bad.
The subscription.getPendingQueueSize() == 0 isn't good enough because
offline durables that are inactive will return 0 if the keep active flag is
false, leading to message loss so that isn't helpful.
So in the end, after like 2 days of back and forth, I think what you have
makes sense after all so i'll approve it as it's the safest option with the new
wildcard check.
--
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
