Core: close MetricsReporter when RESTSessionCatalog is closed#16310
Conversation
|
thanks for catching this @talatuyarer. I think we missed updating this because closing the |
| * 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 ?
There was a problem hiding this comment.
Thanks @singhpk234. Your comment reminded me of the test in CatalogTests. I've moved my standalone test there. Now, we're testing all catalogs, not just the REST catalog.
talatuyarer
force-pushed
the
restcatalog-close-reporter
branch
from
f7bb073 to
154b4af
Compare
|
Do you have any further comments? I believe @talatuyarer would like this fix included in the 1.11 release. If there are no objections, I will merge this PR before RC2 cut tonight. Also cc @aihuaxu |
singhpk234
left a comment
singhpk234
left a comment
There was a problem hiding this comment.
LGTM thanks @talatuyarer !
LGTM. Seems straightforward to me. |
|
Thanks @talatuyarer for the PR! Thanks @nastra @singhpk234 @aihuaxu for the review! |
5 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.