Core: close MetricsReporter when RESTSessionCatalog is closed#16310
Core: close MetricsReporter when RESTSessionCatalog is closed#16310talatuyarer wants to merge 1 commit into
Conversation
| * from the test). | ||
| */ | ||
| public static class CloseTrackingMetricsReporter | ||
| implements org.apache.iceberg.metrics.MetricsReporter { |
There was a problem hiding this comment.
nit: import org.apache.iceberg.metrics.MetricsReporter. Also for java.util.concurrent.atomic.AtomicInteger and org.apache.iceberg.metrics.MetricsReport
There was a problem hiding this comment.
+1 lets avoid inline import !
| .toUpperCase(Locale.US)); | ||
|
|
||
| this.reporter = CatalogUtil.loadMetricsReporter(mergedProps); | ||
| this.closeables.addCloseable(this.reporter); |
There was a problem hiding this comment.
| this.closeables.addCloseable(this.reporter); | |
| this.closeables.addCloseable(reporter); |
| * counter (the reporter is loaded reflectively, so per-instance state is not directly reachable | ||
| * from the test). | ||
| */ | ||
| public static class CloseTrackingMetricsReporter |
There was a problem hiding this comment.
nit: maybe just CloseableMetricsReporter
| CatalogProperties.METRICS_REPORTER_IMPL, | ||
| CloseTrackingMetricsReporter.class.getName())); | ||
|
|
||
| catalog.close(); |
There was a problem hiding this comment.
nit: maybe put the init & closing of the catalog in a try-finally block
|
thanks for catching this @talatuyarer. I think we missed updating this because closing the |
| * from the test). | ||
| */ | ||
| public static class CloseTrackingMetricsReporter | ||
| implements org.apache.iceberg.metrics.MetricsReporter { |
There was a problem hiding this comment.
+1 lets avoid inline import !
| * counter (the reporter is loaded reflectively, so per-instance state is not directly reachable | ||
| * from the test). | ||
| */ | ||
| public static class CloseTrackingMetricsReporter |
There was a problem hiding this comment.
can we just enhance the CatalogTests.CustomMetricsReporter to record the close rather than using new one ?
4 participants
RESTSessionCatalogloaded a customMetricsReporterviaCatalogUtil.loadMetricsReporterbut failed to register it with the closeables group. Consequently,RESTCatalog.close()did not propagate to the reporter, leading to resource leaks for the JVM's lifetime.This change registers the reporter with the catalog's
CloseableGroup, ensuringclose()cascades and mirrorsBaseMetastoreCatalogbehavior.