Skip to content

HDDS-6168: Guard S3G metrics JMX registration#9771

Open
vyalamar wants to merge 1 commit intoapache:masterfrom
vyalamar:hdds-6168-s3g-metrics-jmx
Open

HDDS-6168: Guard S3G metrics JMX registration#9771
vyalamar wants to merge 1 commit intoapache:masterfrom
vyalamar:hdds-6168-s3g-metrics-jmx

Conversation

@vyalamar
Copy link
Contributor

Summary

  • add config toggle to disable S3G metrics (ozone.s3g.metrics.enabled, default true)
  • make S3GatewayMetrics.create idempotent and add JVM shutdown hook to unregister; return null when disabled
  • regression test to assert create() is idempotent

Testing

  • mvn -pl hadoop-ozone/s3gateway -am -DskipITs test (ran locally; completed via cached modules)

@vyalamar
Copy link
Contributor Author

@adoroszlai, @ivanzlenko, and @Russole pls help review.

MetricsSystem ms = DefaultMetricsSystem.instance();
instance = ms.register(SOURCE_NAME, "S3 Gateway Metrics",
new S3GatewayMetrics(conf));
Runtime.getRuntime().addShutdownHook(new Thread(S3GatewayMetrics::unRegister));

Choose a reason for hiding this comment

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

is there a wait loop that lets threads spawned as shutdown hooks to complete?

Copy link
Contributor

@ivanzlenko ivanzlenko Feb 16, 2026

Choose a reason for hiding this comment

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

What is the point of this Shutdown hook anyway? We will unregister metrics on server stop.

Copy link
Contributor

Choose a reason for hiding this comment

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

If you decide that a shutdown hook is needed, please use org.apache.hadoop.ozone.util.ShutdownHookManager.

MetricsSystem ms = DefaultMetricsSystem.instance();
instance = ms.register(SOURCE_NAME, "S3 Gateway Metrics",
new S3GatewayMetrics(conf));
Runtime.getRuntime().addShutdownHook(new Thread(S3GatewayMetrics::unRegister));
Copy link
Contributor

@ivanzlenko ivanzlenko Feb 16, 2026

Choose a reason for hiding this comment

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

What is the point of this Shutdown hook anyway? We will unregister metrics on server stop.

}

@Test
public void testMetricsCreateIsIdempotent() {
Copy link
Contributor

Choose a reason for hiding this comment

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

Would've be lovely to have tests which verify the configuration as well.

Copy link
Contributor

@adoroszlai adoroszlai left a comment

Choose a reason for hiding this comment

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

Thanks @vyalamar for the patch.

Comment on lines +122 to +123
S3GatewayMetrics m1 = S3GatewayMetrics.create(new OzoneConfiguration());
S3GatewayMetrics m2 = S3GatewayMetrics.create(new OzoneConfiguration());
Copy link
Contributor

Choose a reason for hiding this comment

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

Error:  hadoop-ozone/s3gateway/src/test/java/org/apache/hadoop/ozone/s3/metrics/TestS3GatewayMetrics.java:[122,55] cannot find symbol
Error:    symbol:   class OzoneConfiguration
Error:    location: class org.apache.hadoop.ozone.s3.metrics.TestS3GatewayMetrics

https://github.com/vyalamar/ozone/actions/runs/22034226313/job/63664346554#step:16:22

Comment on lines +307 to +308
if (!conf.getBoolean(S3GatewayConfigKeys.OZONE_S3G_METRICS_ENABLED,
S3GatewayConfigKeys.OZONE_S3G_METRICS_ENABLED_DEFAULT)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's cache the result in a static Boolean variable.

if (metricsEnabled == null) {
  metricsEnabled = conf.getBoolean(...);
}
if (!metricsEnabled) {
  return null;
}

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