-
Notifications
You must be signed in to change notification settings - Fork 123
Expose Trino backend state via JMX #782
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
Conversation
Reviewer's GuideThis PR integrates JMX exposure for Trino backend cluster state by injecting an MBeanExporter into BackendStateManager, defining a ClusterStatsJMX bean with Managed annotations and exporting/updating it in updateStates, updates the DI configuration to bind BackendStateManager via Guice, and adds an integration test to verify the new JMX metrics are exposed properly. File-Level Changes
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey there - I've reviewed your changes - here's some feedback:
- Replace the fixed Thread.sleep in testClusterStatsJMX with a polling loop and timeout to avoid flakiness in the test.
- Consider using Map.computeIfAbsent for clusterStatsJMXs in updateStates to streamline the creation and registration logic.
- Add unregister logic for MBeans when a cluster is removed or its stats are no longer tracked to prevent JMX resource leaks.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Replace the fixed Thread.sleep in testClusterStatsJMX with a polling loop and timeout to avoid flakiness in the test.
- Consider using Map.computeIfAbsent for clusterStatsJMXs in updateStates to streamline the creation and registration logic.
- Add unregister logic for MBeans when a cluster is removed or its stats are no longer tracked to prevent JMX resource leaks.
## Individual Comments
### Comment 1
<location> `gateway-ha/src/main/java/io/trino/gateway/ha/router/BackendStateManager.java:68-71` </location>
<code_context>
+
+ public static class ClusterStatsJMX
+ {
+ int runningQueryCount;
+ int queuedQueryCount;
+ int numWorkerNodes;
+ TrinoStatus trinoStatus;
+
+ public ClusterStatsJMX(ClusterStats clusterStats)
</code_context>
<issue_to_address>
**suggestion:** Fields in ClusterStatsJMX should be private for encapsulation.
Making these fields private will prevent external classes from modifying them and improve encapsulation.
```suggestion
private int runningQueryCount;
private int queuedQueryCount;
private int numWorkerNodes;
private TrinoStatus trinoStatus;
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
gateway-ha/src/main/java/io/trino/gateway/ha/router/BackendStateManager.java
Outdated
Show resolved
Hide resolved
8131821 to
54fe806
Compare
54fe806 to
92b1151
Compare
Description
Expose Trino backend state via JMX
Additional context and related issues
JMX looks like this:
I didn't implement unregister logic, because under the current implementation
BackendStateManager.clusterStatswon't delete old Trino cluster. We may need another PR to refactorBackendStateManagerand*Observerclasses to support this.Release notes
(X) Release notes are required, with the following suggested text:
* Expose Trino cluster state via JMX.Summary by Sourcery
Expose Trino backend state via JMX by exporting dynamic ClusterStatsJMX MBeans for each cluster and updating dependency injection to support MBeanExporter
New Features:
Enhancements:
Tests: