[AMQ-9692] Support destination gc sweep of destinations with only wildcard consumers #1484
Conversation
8a4acfd to
d8f0981
Compare
activemq-broker/src/main/java/org/apache/activemq/broker/region/BaseDestination.java
Outdated
Show resolved
Hide resolved
bc16e3a to
6ad24d0
Compare
|
[x] GC of destinations with wildcard only works |
activemq-broker/src/main/java/org/apache/activemq/broker/region/AbstractRegion.java
Show resolved
Hide resolved
activemq-broker/src/main/java/org/apache/activemq/broker/region/BaseDestination.java
Outdated
Show resolved
Hide resolved
|
@mattrpav - The biggest thing now is I think the test coverage is lacking.
I think better test coverage is important because it would be bad to start deleting destinations that should not be deleted as that leads to permanent data loss. |
|
@cshannon new DestinationIsActiveTest adds parameterized tests for logical combinations of: PolicyEntry gc flag combinations vs status of:
There is also a fail-safe check to ensure parameter config is correct by verifying that anytime the normal app consumer is active, the queue is flagging as active. Intellij confirmed 100% branch coverage on those methods (aside from the producer count test) |
| throw new JMSException("Destination: " + destination + " still has an active subscription: " + sub); | ||
| if(sub.isWildcard()) { | ||
| var dest = destinations.get(destination); | ||
| if(dest != null && dest.isGcWithOnlyWildcardConsumers()) { |
There was a problem hiding this comment.
There still seems to be a missing test case because this logic is broken, Intellij flagged it. This continue does nothing as it's in a loop and this will not throw the exception if isGcWithOnlyWildcardConsumers() is false.
This should likely just be rewritten as something like and include a test that would catch a similar mistake in the future.
if (sub.matches(destination) ) {
var dest = destinations.get(destination);
if(!sub.isWildcard() || dest == null || !dest.isGcWithOnlyWildcardConsumers()) {
throw new JMSException("Destination: " + destination + " still has an active subscription: " + sub);
}
}There was a problem hiding this comment.
Would this also need to look for the same for network-only subscribers for consistency? Or better yet perhaps re-use the isActive() or canGC() methods?
Note: This method is invoked by the RegionBroker gc check after confirming the expiration rules apply for the given destination.
In the case of GC, this is a repeat of the check.
In the case of an admin operation (ie via CLI or JMX) this is the only check to prevent deleting a destination w/ active subscribers.
There was a problem hiding this comment.
isActive() and canGC() are a bit different because they also check for active producers. I'm not entirely sure why the method to remove was written not to care about producers, maybe an oversight, but changing that now would be a behavior change that could cause some side effects that are unexpected. Re-using logic is good but maybe it just needs to be subdivided to only check for active consumers (that method already exists and isActive() calls it now).
As you pointed out, the RegionBroker calls this to delete the destination but already after the gc check so we know there are no active consumer/producers. So the real question is whether or not we care about blocking a manual deletion through JMX if there are active producers. It seems odd to not check that but changing that now as I said is a behavior change that could break someone's use case. (I suppose i could see someone wanting to block deletes with active consumers but not producers).
Your point about it not checking the gcWithNetworkConsumers flag is valid and probably a bug. In fact it proves the point that we should be sharing the logic because the GC automated check will check it but not the manual deletion.
There was a problem hiding this comment.
There's a long list of things that are like this where we may want to revisit for AMQ 7 where we want to change default behavior or settings but it's technically a breaking change so a major version is more appropriate. I guess whether or not we change this to check active producers depends on if we consider it a bug or not.
If we change it I would think by default this would share the same logic as the isActive and GC check and that should block removal by default even by an admin through JMX/console etc to prevent mistakes. But, if we do that we could add a force flag that would allow an administrator to force delete, but all of this is kind of out scope of the work for this.
No description provided.