[AMQ-9548] Add BrokerView attributes to retrieve number of total, managed, and temporary queues and topics

[kenliao94]

What problems does this PR solve?
Currently, to get the total number of queues and topics on the broker using JMX, one needs to query the attribute values of Queues and Topics on the broker MBean then post-process it (finding the length of returned array) to get that data. This can be resource intensive (unnecessary copy of the data) on the customer application using JMX or Jolokia if user has tens or thousands of destinations and all the user wants is the number. Hence this PR added several attributes so customer application can just query the total number of unsuppressed, total, and temporary without getting the list of topics and queues first.

Why is it beneficial to merge into ActiveMQ?
This will be beneficial for users with high number of destinations. They can now monitor the total number of queues and topics in a more performant way.

How do you make sure this PR is well tested?

• Added unit test in the PR
• Tested with Jolokia (cross-checked with the web console) [see attachment below]
• Tested with Jconsole (cross-checked with the web console) [see attachment below]

[cshannon]

@jbonofre and @mattrpav -

I took a look at this and I agree with Matt for all the points he made. The suppression configuration here is because of performance reasons. You are suppressing the returned queue metrics over JMX to not overload things. That doesn't mean the queues don't exist or are not valid so I don't think it makes sense to hide suppressed queues from the metrics. On the other hand I get JB's point about it could be a little confusing if getQueues() is returning a different number because of the suppression.

So if I had to pick one I would agree with @mattrpav and say we should return total queues. However, why not just provide both and call it a day? Just provide both a total count for the broker of the over all queue count and also an un-suppressed count metric.

We could have something like getTotalQueueCount() and getUnsupressedQueueCount() or something like that. The total queues just can just use the map size with is a fast constant operation as @mattrpav pointed out. For suppressed queues you could iterate over the queues in the map and count them to avoid object allocation on the broker side, or we could just keep a counter that keeps a running total on queue adds/deletes.

[cshannon]

Also the naming doesn't matter, can pick whatever makes sense. The point is just provide both metrics as they could both be useful but make sure we compute them without a bunch of memory allocation

[jbonofre]

Ok fair enough. I propose to add the two operations as I'm sure users will need both.

[jbonofre]

New MBean operation names look good to me. @mattrpav do you mind to take a new look and approve the PR ?

[kenliao94]

New MBean operation names look good to me. @mattrpav do you mind to take a new look and approve the PR ?

Hi @mattrpav, do you have any more concerns about this PR?

[mattrpav]

1. Please use the more appropriate assert* methods.

2. These tests are dodgy. The value expect is 1 and the same across the different types.

3. Multiple commits need to be squashed for this change

[cshannon]

I'll just point out that squashing commits is fine but it is also not necessary. I know I've mentioned before but I prefer it if people do not force push and just push new commits so you can see the difference easier between versions. It's easy to do a squash merge several commits at the end into 1 commit when merging back to main.

[kenliao94]

Done. Thanks for the feedback @mattrpav

[kenliao94]

Hey @jbonofre do you think we should put this in 6.1.5?

[cshannon]

Hey @jbonofre do you think we should put this in 6.1.5?

No, we should not be adding changes like this to a bug fix release. Releases like 6.1.5 or 6.1.6 should just be bug fixes, security fixes, minor dependency updates, etc.

There was a discussion started on SEMVER on the dev list that we probably need to get back to so that it's more clear what we change in different versions.

If new features are being developed quickly and we end up with lots of stuff to release we can always quickly do releases. There doesn't have to be a long wait in between 6.2.0 and 6.3.0 for example if there's a new JMS 2.0 feature that is ready. Artemis already does this, if you look at past Artemis releases, they release relatively frequently.

[jbonofre]

No it should be for
6.2.0, not 6.1.5 as it's not a bug fix.

[jbonofre]

This PR looks good to go in 6.2.0 for me.

@mattrpav you still have "change request" on the PR but I think @kenliao94 addressed your comments. Can you confirm ? Thanks !

[mattrpav]

@kenliao94 The changes look good, thanks!

I want to take one last minute to discuss the naming, b/c these are public interfaces and once go out we are living with them for a long time.

I think using a term that says what 'TotalUnsuppressedQueueCount' is would be better than what it isn't.

How about 'TotalManagedQueueCount'? -- I think this terminology better describes what that number means.

[kenliao94]

Thanks for the comment. I caught a bug. The TotalManaged counts suppressed one as well. I,E safeGetBroker().getTopicViews().size() returns suppressed ones as well
However, when I do more deep dive, It turns out that there is no map that stores managed destinations. The getQueues() and getTopics() are doing the filtering everytime it is called because of that. When a MBean is registered, it internally invokes isAllowedToRegister to check if a given MBean can be registered and register it if allowed. But instead of storing different maps categorized by destination, it is a single map of all registered MBeans. Hence, to get the total number of managed queue or topic, we need to iterate a list of queue or topic and invoke isAllowedToRegister again every time (similar to how onlyNonSuppressed work, it is not retrieving a cached value, but iterate over a set and check everytime).

Furthermore, why safeGetBroker().getTopicViews().size() didn't work? Because registerDestination didn't check if that ObjectName is allow to register before putting it into the map

    protected void registerDestination(ObjectName key, ActiveMQDestination dest, DestinationView view) throws Exception {
        if (dest.isQueue()) {
            if (dest.isTemporary()) {
                temporaryQueues.put(key, view);
            } else {
                queues.put(key, view);
            }
        } else {
            if (dest.isTemporary()) {
                temporaryTopics.put(key, view);
            } else {
                topics.put(key, view);
            }
        }
        try {
            if (AsyncAnnotatedMBean.registerMBean(asyncInvokeService, mbeanTimeout, managementContext, view, key) != null) {
                registeredMBeans.add(key);
            }
        } catch (Throwable e) {
            LOG.warn("Failed to register MBean {}", key);
            LOG.debug("Failure reason: ", e);
        }
    }

For now, I agree with your suggestion on exposing the APIs and what do that suppose to mean. I changed the name of those metrics "NonSuppressed" -> "Managed" and added the testcase to test for suppressed destination. The only thing I rollback to, is that I need to first generate the array first and reads the size, the inefficiency you initially pointed out.

  @Override
  public int getTotalManagedTopicsCount() {
      return safeGetBroker().getTopicsNonSuppressed().length;
  }

That said, I can follow up with another PR that changes the way how the ManagementContext stores the registered destination, such that getTotalManagedQueuesCount and getTotalManagedTopicsCount can be done by reading a map size and it won't be a API / breaking change, so it can be done in a patch version. WDYT? @mattrpav

[mattrpav]

LGTM

@kenliao94 good stuff! Thanks for updating the test to include suppressed destinations.

Since this is the broker mbean, and not a bean used by 1,000s to 10,000s of objects (like queue, topic or connection), I think the array inefficiency is negligible to go forward.

edit: reminder to squash commits on merge

[jbonofre]

That looks good to me and reasonable.