cshannon commented on code in PR #1484:
URL: https://github.com/apache/activemq/pull/1484#discussion_r2729113503
##########
activemq-broker/src/main/java/org/apache/activemq/broker/region/AbstractRegion.java:
##########
@@ -260,16 +260,21 @@ protected List<Subscription>
addSubscriptionsForDestination(ConnectionContext co
}
@Override
- public void removeDestination(ConnectionContext context,
ActiveMQDestination destination, long timeout)
- throws Exception {
-
+ public void removeDestination(ConnectionContext context,
ActiveMQDestination destination, long timeout) throws Exception {
// No timeout.. then try to shut down right way, fails if there are
// current subscribers.
if (timeout == 0) {
for (Iterator<Subscription> iter =
subscriptions.values().iterator(); iter.hasNext();) {
Subscription sub = iter.next();
if (sub.matches(destination) ) {
- throw new JMSException("Destination: " + destination + "
still has an active subscription: " + sub);
+ if(sub.isWildcard()) {
+ var dest = destinations.get(destination);
+ if(dest != null &&
dest.isGcWithOnlyWildcardConsumers()) {
Review 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.
--
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
