[mattrpav]

Background

Garbage collecting destinations allows for a better alternative from temp-queue and temp-topic usage, since those rely on randomly generated destination names and are not subject to fine-grained permissions. In that, TempQueues and TempTopics are best to be used by applications that have dedicated brokers.

A one-time use named queue can be created in a namespace for a shared broker cluster and provide for fine-grained security and self-service.

Scope

Destination Type	In scope
Queue	Yes
Topic (non-durable)	Yes
Topic (durable	No

Current

Current destination gc sees a wildcard-only consumer as a consumer and will not reap that destination. Adding support for gc'n wildcard-only consumers means a pattern where producers send to one-time use queues and consumers use wildcard consumers may still have the one-time queues deleted.

Benefits

One-time use queues paired with wildcard consumers are a better pattern over using priority or message groups as there is no longer a front-of-line problem or the need to have permanent queues or additional indexing added.

[mattrpav]

[x] GC of destinations with wildcard only works
[x] Tested lifecycle of wildcard consumer working when matching queue is added, gc'd and re-added. Messages are consumed properly.

[jbonofre]

LGTM, thanks !

[cshannon]

@mattrpav - The biggest thing now is I think the test coverage is lacking.

1. Some of the new methods could be unit tested . The new hasActiveConsumers() and canGcConsumer predicate can probably be tested in isolation as they are protected so maybe with mocks or something.
2. I think we need to double check there is sufficient test cover for the different combinations of the gcWithOnlyWildcardConsumers and gcWithOnlyWildcardConsumers flags being set. You added a test if gcWithOnlyWildcardConsumers is true but we should also add a quick test where both flags are true (easy enough to change in the policy and can probably just reuse the same test). We should also verify there's a test somewhere that still works for gc network consumers flag (i would hope so).
3. Verify that there's good GC test coverage with both flags false as the default, i would expect there should already be some GC tests but I haven't looked in a while.

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.

[mattrpav]

@cshannon new DestinationIsActiveTest adds parameterized tests for logical combinations of:

PolicyEntry gc flag combinations vs status of:

1. Normal consumer
2. Network consumer
3. Wildcard consumer

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)

[cshannon]

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);
    } 
}

[mattrpav]

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.

[cshannon]

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.

[cshannon]

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.

[jbonofre]

I guess we would like this fix included in 5.19.2 right ?

@cshannon do we make a new pass on this PR or is it good to go ?

[cshannon]

I guess we would like this fix included in 5.19.2 right ?

@cshannon do we make a new pass on this PR or is it good to go ?

This still needs a little more work so maybe we can hold off if we are ready to do releases now

[jbonofre]

@mattrpav if you want me to include this PR in the 5.19.2 release, can you please update ?
Else, I will move forward with the release and we can include this PR in the next one.

[github-actions]

This PR is stale because it has been open 60 days with no activity. Remove stale label or comment or this will be closed in 5 days.

[mattrpav]

Updated tests include non-durable topics. Durable topics are not included by design. Also, added a 'last minute' check to make sure destinations don't get busy from the time of being marked and removed.

[cshannon]

I found several things to change, comments in line