tanmayrauth commented on code in PR #1430:
URL: https://github.com/apache/iceberg-go/pull/1430#discussion_r3599426020
##########
catalog/hive/hive.go:
##########
@@ -94,8 +98,10 @@ func (c *Catalog) CatalogType() catalog.Type {
return catalog.Hive
}
+// Close releases the Hive client and the catalog's metrics reporter, joining
+// any errors so one failing close does not skip the other.
func (c *Catalog) Close() error {
- return c.client.Close()
+ return errors.Join(c.client.Close(), c.reporter.Close())
Review Comment:
Fixed, though pure sequencing wouldn't cover it — errors.Join already
evaluates both regardless of error, so only a panic skips the second close.
Wrapped each in a closeIsolated helper that recovers into an error and joins
(same shape as compositeReporter.Close). Pinned by a test where the client
panics on Close and the reporter still closes.
##########
catalog/glue/glue.go:
##########
@@ -241,6 +245,12 @@ func (c *Catalog) CatalogType() catalog.Type {
return catalog.Glue
}
+// Close releases the catalog's metrics reporter. The Glue catalog does not own
+// the lifetime of the AWS clients it was configured with, so only the reporter
+// is released. Callers holding a [catalog.Catalog] can reach this via an
+// io.Closer type assertion.
+func (c *Catalog) Close() error { return c.reporter.Close() }
Review Comment:
Went with the catalog.Closer optional-interface idiom you suggested rather
than deferring — documented it PurgeableTable-style with a defer usage example
and asserted it on all five catalogs, so catalog.Load callers get Close via one
documented assertion. Left it off the Catalog interface itself since that's
the breaking change; happy to file a follow-up if you'd still want the
promotion tracked.
##########
metrics/cached_reporter.go:
##########
@@ -0,0 +1,65 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+package metrics
+
+import "sync"
+
+// CachedReporter builds a [Reporter] from catalog properties once and caches
+// it, so a catalog holds a single reporter for its lifetime — matching Java's
+// per-catalog MetricsReporter — rather than constructing a fresh one on every
+// table load or commit. That per-operation construction is what makes a
+// stateful reporter (an HTTP-backed one holding a shared client or a
background
+// dispatch worker) leak: a new one per load with no owner to close it.
+//
+// The zero value is ready to use and safe for concurrent use. Close releases
+// the built reporter, giving a catalog a single place to clean up at shutdown.
+type CachedReporter struct {
+ mu sync.Mutex
+ built bool
+ rep Reporter
+ err error
+}
+
+// Get returns the cached reporter, building it from props on the first call
via
+// [FromProperties]. The first call's result — reporter and error — is cached
and
+// returned to every later caller; props supplied on subsequent calls is
ignored,
+// because a catalog's reporter configuration does not change over its
lifetime.
+func (c *CachedReporter) Get(props map[string]string) (Reporter, error) {
+ c.mu.Lock()
+ defer c.mu.Unlock()
+ if !c.built {
+ c.rep, c.err = FromProperties(props)
+ c.built = true
+ }
+
+ return c.rep, c.err
+}
+
+// Close closes the built reporter, if one was ever built, and is a no-op
+// otherwise (including when Get was never called or returned an error). Close
is
+// expected at catalog shutdown, after operations have quiesced; it is safe for
+// concurrent use, but a Get racing a Close has no defined ordering.
+func (c *CachedReporter) Close() error {
+ c.mu.Lock()
+ defer c.mu.Unlock()
+ if c.rep != nil {
Review Comment:
Close now nils the reporter after closing, so it's idempotent and a
post-Close Get returns NopReporter{} rather than a released reporter.
Documented the typed-nil case as a Factory contract (must return a usable
reporter or an error) instead of a reflect check. Added tests for both.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
