laskoviymishka commented on code in PR #1430:
URL: https://github.com/apache/iceberg-go/pull/1430#discussion_r3558586687
##########
table/table.go:
##########
@@ -157,6 +168,7 @@ func (t *Table) Refresh(ctx context.Context) error {
t.fsF = fresh.fsF
t.metadataLocation = fresh.metadataLocation
t.planner = fresh.planner
+ t.reporter = fresh.reporter
Review Comment:
Refresh unconditionally copies `fresh.reporter`, so any reporter a caller
injected via `WithMetricsReporter` gets replaced on the next refresh. Since
commit retry loops call Refresh, a user-configured reporter silently reverts to
the catalog-derived one (nop, in most setups) after the first cycle.
I'd only inherit the fresh reporter when the caller hasn't set one:
```go
if _, isNop := t.reporter.(metrics.NopReporter); isNop {
t.reporter = fresh.reporter
}
```
Either that or document the reset explicitly — but either way this needs a
test. Right now nothing covers reporter survival across Refresh, and it's the
invariant most likely to regress. wdyt?
##########
metrics/registry_test.go:
##########
@@ -0,0 +1,79 @@
+// 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 (
+ "testing"
+
+ "github.com/stretchr/testify/assert"
+ "github.com/stretchr/testify/require"
+)
+
+func TestFromProperties(t *testing.T) {
+ t.Run("absent name yields NopReporter", func(t *testing.T) {
+ r, err := FromProperties(nil)
+ require.NoError(t, err)
+ assert.IsType(t, NopReporter{}, r)
+ })
+
+ t.Run("empty name yields NopReporter", func(t *testing.T) {
+ r, err := FromProperties(map[string]string{ReporterImplKey: ""})
+ require.NoError(t, err)
+ assert.IsType(t, NopReporter{}, r)
+ })
+
+ t.Run("nop name yields NopReporter", func(t *testing.T) {
+ r, err := FromProperties(map[string]string{ReporterImplKey:
"nop"})
+ require.NoError(t, err)
+ assert.IsType(t, NopReporter{}, r)
+ })
+
+ t.Run("logging name yields LoggingReporter", func(t *testing.T) {
+ r, err := FromProperties(map[string]string{ReporterImplKey:
"logging"})
+ require.NoError(t, err)
+ assert.IsType(t, &LoggingReporter{}, r)
+ })
+
+ t.Run("unknown name is an error", func(t *testing.T) {
+ _, err := FromProperties(map[string]string{ReporterImplKey:
"does-not-exist"})
+ assert.Error(t, err)
+ })
+}
+
+func TestRegister(t *testing.T) {
+ t.Run("factory is selectable via FromProperties", func(t *testing.T) {
+ Register("test-custom", func(props map[string]string)
(Reporter, error) {
Review Comment:
This mutates the process-global registry and never cleans up, so `go test
-count=2 ./metrics/` panics on the duplicate Register — and the registry can't
be exercised in isolation.
I'd make the registry testable: either expose a `newRegistry()` that tests
use for a fresh instance, or register with `t.Cleanup(func(){ Deregister(...)
})`. The struct-with-methods shape is nicer long-term since it also removes the
global-mutable-state smell, but a cleanup hook is the minimal fix. Thoughts?
##########
catalog/hive/hive.go:
##########
@@ -149,12 +150,18 @@ func (c *Catalog) LoadTable(ctx context.Context,
identifier table.Identifier) (*
return nil, fmt.Errorf("failed to get metadata location: %w",
err)
}
+ reporter, err := metrics.FromProperties(c.opts.props)
Review Comment:
We call `FromProperties` on every load/register across all five catalogs, so
each table op builds a fresh reporter instance. Java keeps a single
lazily-created reporter per catalog. For the built-in nop/logging reporters
this is fine since they're stateless, but a stateful reporter — one holding a
connection, buffer, or pool, or meant to aggregate across calls — gets
multiplied per table op and siloed per table.
Not blocking for this PR since the built-ins are stateless and there's no
Closer path yet, but I'd want us to decide whether reporter lifetime is
per-catalog or per-table before third-party stateful reporters show up. A
per-catalog cached reporter would match Java. wdyt?
##########
catalog/sql/sql.go:
##########
@@ -434,12 +435,18 @@ func (c *Catalog) LoadTable(ctx context.Context,
identifier table.Identifier) (*
return nil, fmt.Errorf("%w: %s, metadata location is missing",
catalog.ErrNoSuchTable, identifier)
}
+ reporter, err := metrics.FromProperties(c.props)
Review Comment:
Bare `return nil, err` here drops the operation context — everywhere else in
these catalog files we wrap with `fmt.Errorf("failed to ...: %w")`. A user who
typos the reporter name gets `metrics: unknown reporter "foo"` with no hint
which catalog or table triggered it.
I'd wrap it: `fmt.Errorf("failed to initialize metrics reporter: %w", err)`.
Same pattern repeats in the glue/hadoop/hive/rest sites.
##########
catalog/sql/sql_test.go:
##########
@@ -425,6 +426,77 @@ func (s *SqliteCatalogTestSuite)
TestCreateTableDefaultSortOrder() {
}
}
+func (s *SqliteCatalogTestSuite) TestMetricsReporterWiring() {
+ ctx := context.Background()
+
+ newCatMemory := func(extra iceberg.Properties) *sqlcat.Catalog {
+ props := iceberg.Properties{
+ "uri": ":memory:",
+ sqlcat.DriverKey: sqliteshim.ShimName,
+ sqlcat.DialectKey: string(sqlcat.SQLite),
+ "type": "sql",
+ "warehouse": "file://" + s.warehouse,
+ }
+ maps.Copy(props, extra)
+ cat, err := catalog.Load(ctx, "default", props)
+ s.Require().NoError(err)
+
+ return cat.(*sqlcat.Catalog)
+ }
+
+ createTable := func(cat *sqlcat.Catalog) (*table.Table,
table.Identifier) {
+ tblID := s.randomTableIdentifier()
+ s.Require().NoError(cat.CreateNamespace(ctx,
catalog.NamespaceFromIdent(tblID), nil))
+ tbl, err := cat.CreateTable(ctx, tblID, tableSchemaNested)
+ s.Require().NoError(err)
+
+ return tbl, tblID
+ }
+
+ s.Run("configured reporter reaches created and loaded table", func() {
+ cat := newCatMemory(iceberg.Properties{metrics.ReporterImplKey:
"logging"})
+ created, tblID := createTable(cat)
+ s.IsType(&metrics.LoggingReporter{}, created.MetricsReporter())
+
+ loaded, err := cat.LoadTable(ctx, tblID)
+ s.Require().NoError(err)
+ s.IsType(&metrics.LoggingReporter{}, loaded.MetricsReporter())
+
+ // Scans inherit the table's reporter.
+ s.IsType(&metrics.LoggingReporter{}, loaded.Scan().Reporter())
+ })
+
+ s.Run("default is the no-op reporter", func() {
+ cat := newCatMemory(nil)
+ created, _ := createTable(cat)
+ s.IsType(metrics.NopReporter{}, created.MetricsReporter())
+ })
+
+ s.Run("unknown reporter name fails table load", func() {
+ // Create with a well-configured catalog, then reopen the same
+ // on-disk DB with a bad reporter name so the failure is
isolated to
+ // LoadTable rather than the create path.
+ good := s.getCatalogSqlite()
Review Comment:
This leans on `s.getCatalogSqlite()`/`s.catalogUri()` sharing on-disk state
with the sibling suite setup, which makes the test order-dependent and fragile.
I'd create and reopen the DB within the test itself so the unknown-reporter
failure is fully self-contained.
##########
catalog/glue/glue.go:
##########
@@ -279,13 +280,18 @@ func (c *Catalog) RegisterTable(ctx context.Context,
identifier table.Identifier
}
// Load the metadata file to get table properties
ctx = utils.WithAwsConfig(ctx, c.awsCfg)
+ reporter, err := metrics.FromProperties(c.props)
Review Comment:
This reporter block is dead — the temporary table built here via
`NewFromLocation` is discarded, and RegisterTable returns `c.LoadTable(...)` at
the tail, which re-resolves the reporter through `convertGlueToIceberg`. So we
resolve twice and only the second one survives.
I'd drop the `FromProperties` + `WithMetricsReporter` here and let the tail
LoadTable own the wiring, matching how sql.CreateTable delegates. As written
it's harmless but confusing, and a future `NewFromLocation` path that skips
LoadTable would silently lose the reporter.
##########
metrics/registry.go:
##########
@@ -0,0 +1,89 @@
+// 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 (
+ "fmt"
+ "sync"
+)
+
+// ReporterImplKey is the catalog/table property that selects a registered
+// reporter by name (e.g. "logging"). It is the Go analogue of Java's
+// metrics-reporter-impl; Go uses a name→factory registry rather than
+// reflection over a class name.
+const ReporterImplKey = "metrics-reporter-impl"
+
+// Built-in reporter names usable as the value of [ReporterImplKey].
+const (
+ reporterNameNop = "nop"
Review Comment:
Users have to pass `"nop"`/`"logging"` as bare string literals in their
property maps, with no compile-time signal if they fumble one. Since
`ReporterImplKey` is already exported, I'd export these too — `ReporterNameNop`
/ `ReporterNameLogging` — so callers get a symbol to reference. wdyt?
##########
catalog/rest/rest.go:
##########
@@ -834,12 +835,18 @@ func (r *Catalog) tableFromResponse(_ context.Context,
identifier []string, meta
fsF = iceio.LoadFSFunc(config, loc)
}
+ reporter, err := metrics.FromProperties(config)
Review Comment:
Every other catalog resolves the reporter from its own catalog props, but
here we're passing the fully-merged `config` — `r.props` plus the table
metadata properties plus the server-vended `ret.Config` plus creds. So a
`metrics-reporter-impl` coming back from the server, or set as a table
property, can shadow what the client configured, and last-layer-wins is hard to
reason about. Worse, an unknown name in any of those layers turns LoadTable
into a hard client-side error.
I'd resolve from `r.props` here to match the other catalogs:
```go
reporter, err := metrics.FromProperties(r.props)
```
If server-vended reporter selection is actually intended, that's a real
design decision worth calling out explicitly rather than falling out of the
merge order. wdyt?
--
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]
