laskoviymishka commented on code in PR #1430: URL: https://github.com/apache/iceberg-go/pull/1430#discussion_r3590558590
########## 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: Since the whole point of the io.Closer work is the first stateful (HTTP-backed) reporter, I'd tighten this a touch. We don't reset `c.rep` after closing, so a second `Close()` re-closes the underlying reporter — the contract says Close is called once, and a user reporter isn't obliged to be idempotent. Niling `c.rep` here makes Close idempotent and also stops a post-Close `Get()` from handing back an already-closed reporter. The same guard is interface-identity, so a factory returning a typed-nil (`(*Custom)(nil)`) would slip past `c.rep != nil` and panic in Close unrecovered — worth a line on the factory contract, or a defensive check. wdyt? ########## catalog/hadoop/hadoop.go: ########## @@ -549,12 +557,18 @@ func (c *Catalog) CreateTable(ctx context.Context, ident table.Identifier, sc *i c.writeVersionHint(ident, version) + reporter, err := c.reporter.Get(c.props) Review Comment: This is the one I'd still hold on — it's zeroshade's ordering thread, and I think it's actually a bit worse than the partial-success framing we started with. The first `Get(c.props)` fires after `commitMetadataFile` and `writeVersionHint` have already written the table to disk, so an invalid `metrics-reporter-impl` turns a create that physically succeeded into a returned error, and the retry then hits `ErrTableAlreadyExists`. But because `CachedReporter` caches that first error, every later `LoadTable`/`loadTable`/`CreateTable` on the same catalog returns it too — one typo wedges the whole catalog for all table ops until it's reconstructed. Glue has the identical shape through `convertGlueToIceberg`. `CachedReporter` is built exactly for resolving once up front — I'd call `c.reporter.Get(c.props)` in `NewCatalog` (or validate before the first mutation) so a reporter error can't masquerade as a failed create and can't poison later ops. wdyt? ########## 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: The comment says one failing close doesn't skip the other, but that only holds for errors — `errors.Join(c.client.Close(), c.reporter.Close())` evaluates both args before the call, so if `c.client.Close()` panics the reporter never closes and leaks. Sequencing into locals first (`clientErr := ...; reporterErr := ...`) keeps the guarantee the comment promises. ########## 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: The reporter half of the round-2/3 `Close()` ask landed nicely — but this is where the catalog half is still missing: `Close()` is on every concrete catalog, not on `catalog.Catalog`, and `catalog.Load` hands back the interface. So the common caller can't `defer cat.Close()` without a type assertion, and the leak becomes the default outcome for the first stateful reporter. I don't think adding `Close()` to the interface has to block this PR — you're right that it's a broader breaking change. But I'd like a tracked follow-up with an owner before we merge, either adding it to the interface or a documented `catalog.Closer` optional-interface idiom like `PurgeableTable`. Can we file that? -- 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]
