-
Notifications
You must be signed in to change notification settings - Fork 76
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
Export ClusterStats to JMX #502
base: main
Are you sure you want to change the base?
Conversation
We can test it using diff --git a/gateway-ha/src/test/java/io/trino/gateway/ha/TestGatewayHaMultipleBackend.java b/gateway-ha/src/test/java/io/trino/gateway/ha/TestGatewayHaMultipleBackend.java
index af680a6..f124e9d 100644
--- a/gateway-ha/src/test/java/io/trino/gateway/ha/TestGatewayHaMultipleBackend.java
+++ b/gateway-ha/src/test/java/io/trino/gateway/ha/TestGatewayHaMultipleBackend.java
@@ -338,6 +338,22 @@ final class TestGatewayHaMultipleBackend
assertThat(callbackResponse.code()).isEqualTo(500);
}
+ @Test
+ void testClusterStatsJMX()
+ throws Exception
+ {
+ Thread.sleep(5000); // Wait for first health check to run
+ Request request = new Request.Builder()
+ .url("http://localhost:" + routerPort + "/metrics")
+ .get()
+ .build();
+ Response response = httpClient.newCall(request).execute();
+ String body = response.body().string();
+ System.out.println(body);
+ assertThat(body).contains("trino1_Healthy");
+ assertThat(body).contains("trino2_Healthy");
+ }
+
@AfterAll
void cleanup()
{
diff --git a/gateway-ha/src/test/resources/test-config-template.yml b/gateway-ha/src/test/resources/test-config-template.yml
index beafe30..80ab3f3 100644
--- a/gateway-ha/src/test/resources/test-config-template.yml
+++ b/gateway-ha/src/test/resources/test-config-template.yml
@@ -11,6 +11,11 @@ dataStore:
modules:
- io.trino.gateway.ha.module.HaGatewayProviderModule
+ - io.trino.gateway.ha.module.ClusterStateListenerModule
+ - io.trino.gateway.ha.module.ClusterStatsMonitorModule
+
+managedApps:
+ - io.trino.gateway.ha.clustermonitor.ActiveClusterMonitor
extraWhitelistPaths:
- '/v1/custom.*'
@@ -23,3 +28,6 @@ gatewayCookieConfiguration:
oauth2GatewayCookieConfiguration:
deletePaths:
- "/custom/logout"
+
+monitor:
+ taskDelaySeconds: 1 |
@choiwaiyiu I just skimmed and this looks good overall. Can you add a test as suggested by @oneonestar? Please update the description to suggest a simple release note as well. |
@willmostly @oneonestar |
@willmostly @oneonestar @choiwaiyiu What's the current status of this PR? |
} | ||
|
||
@PostConstruct | ||
public synchronized void updateExport() |
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.
Is it possible to make updateExport()
takes List<ClusterStats>
from ClusterStatsObserver
? In that case we don't have to inject BackendStateManager and don't need to add getAllBackendStates()
.
ClusterStatsJMX clusterStatsJMX = backendStates.get(clusterId); | ||
clusterStatsJMX.setFrom(clusterStats); |
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.
ClusterStatsJMX clusterStatsJMX = backendStates.get(clusterId); | |
clusterStatsJMX.setFrom(clusterStats); | |
backendStates.get(clusterId).setFrom(clusterStats); |
Exports
ClusterStats
to JMX.Release notes
( x) This is not user-visible or is docs only, and no release notes are required.