Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[AMQ-9548] Add attributes to retrieve number of total, non-suppressed and temporary queues and topics #1288

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

kenliao94
Copy link
Contributor

@kenliao94 kenliao94 commented Aug 27, 2024

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]

image
image
image

@jbonofre jbonofre self-requested a review October 23, 2024 13:50
@cshannon
Copy link
Contributor

cshannon commented Oct 23, 2024

@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
Copy link
Contributor

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
Copy link
Member

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

@kenliao94 kenliao94 changed the title [AMQ-9548] Add attributes to display total number of queues and topics [AMQ-9548] Add attributes to retrieve number of total, non-suppressed and temporary queues and topics Oct 26, 2024
Copy link
Member

@jbonofre jbonofre left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@kenliao94
Copy link
Contributor Author

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?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants