laskoviymishka commented on code in PR #1430: URL: https://github.com/apache/iceberg-go/pull/1430#discussion_r3577349518
########## metrics/registry.go: ########## @@ -0,0 +1,104 @@ +// 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" + ReporterNameLogging = "logging" +) + +// Factory builds a Reporter from configuration properties. The same property +// map that selected the reporter is passed in, so a factory may read its own +// configuration keys. +type Factory func(props map[string]string) (Reporter, error) + +var ( + registryMu sync.RWMutex + registry = map[string]Factory{} +) + +func init() { + Register(ReporterNameNop, func(map[string]string) (Reporter, error) { return NopReporter{}, nil }) + Register(ReporterNameLogging, func(map[string]string) (Reporter, error) { + return NewLoggingReporter(nil), nil + }) +} + +// Register makes a reporter factory available under name. It panics if name is +// empty or already registered, mirroring database/sql.Register — registration +// is expected to happen once, from package init. +func Register(name string, factory Factory) { + if name == "" { + panic("metrics: Register called with empty name") + } + if factory == nil { + panic("metrics: Register called with nil factory") + } + registryMu.Lock() + defer registryMu.Unlock() + if _, dup := registry[name]; dup { + panic("metrics: Register called twice for " + name) + } + registry[name] = factory +} + +// Deregister removes a previously registered reporter factory. It is a no-op if +// name is not registered. This exists primarily so tests can register a factory +// and undo it via t.Cleanup, keeping the process-global registry re-runnable +// under go test -count=N. +func Deregister(name string) { Review Comment: Since this is public API and we're stabilizing it now — `Deregister` will happily remove the built-in `nop`/`logging` factories, and because `Register` panics on duplicate, nothing can put them back for the rest of the process. So a stray `Deregister("nop")` permanently breaks `metrics-reporter-impl=nop` everywhere. I'd guard the two built-in names, or at least call the hazard out in the godoc. wdyt? ########## table/table.go: ########## @@ -603,7 +628,7 @@ func (t Table) doCommit(ctx context.Context, updates []Update, reqs []Requiremen deleteOldMetadata(fs, t.metadata, newMeta) - return New(t.identifier, newMeta, newLoc, t.fsF, t.cat), nil + return New(t.identifier, newMeta, newLoc, t.fsF, t.cat, WithMetricsReporter(t.MetricsReporter())), nil Review Comment: This is the one thing I'd fix before merge, and it's a consequence of the round-2 change that carries the reporter through `doCommit` — good change, but `t.MetricsReporter()` is never nil, so `WithMetricsReporter(t.MetricsReporter())` flips `reporterSet` to true unconditionally. A table that was riding the catalog default now looks caller-set after its first commit, and every subsequent `Refresh` stops inheriting the catalog reporter — the exact invariant `TestRefreshInheritsCatalogReporterWhenUnset` pins, except that test only covers create+Refresh, not create+Commit+Refresh. I'd propagate the state directly instead of round-tripping through the accessor — a private `withReporterState(t.reporter, t.reporterSet)` option that copies both fields, so a defaulted table stays defaulted across a commit. Same shape at `StagedTable`, though that one's short-lived so it barely matters in practice. A create+Commit+Refresh test would lock it down. wdyt? ########## table/metrics_wiring_test.go: ########## @@ -0,0 +1,161 @@ +// 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 table + +import ( + "context" + "testing" + + "github.com/apache/iceberg-go" + iceio "github.com/apache/iceberg-go/io" + "github.com/apache/iceberg-go/metrics" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestTableMetricsReporterDefaultsToNop(t *testing.T) { + tbl := New(nil, nil, "", nil, nil) + assert.IsType(t, metrics.NopReporter{}, tbl.MetricsReporter()) +} + +func TestWithMetricsReporter(t *testing.T) { + rep := &metrics.InMemoryReporter{} + tbl := New(nil, nil, "", nil, nil, WithMetricsReporter(rep)) + assert.Same(t, rep, tbl.MetricsReporter()) + + // A nil reporter option is ignored; the default no-op reporter is kept. + tblNil := New(nil, nil, "", nil, nil, WithMetricsReporter(nil)) + assert.IsType(t, metrics.NopReporter{}, tblNil.MetricsReporter()) +} + +func TestScanInheritsReporterFromTable(t *testing.T) { + rep := &metrics.InMemoryReporter{} + tbl := New(nil, nil, "", nil, nil, WithMetricsReporter(rep)) + + scan := tbl.Scan() + assert.Same(t, rep, scan.Reporter()) +} + +func TestScanReporterDefaultsToNop(t *testing.T) { + tbl := New(nil, nil, "", nil, nil) + assert.IsType(t, metrics.NopReporter{}, tbl.Scan().Reporter()) +} + +func TestWithReporterOverridesScan(t *testing.T) { + tableRep := &metrics.InMemoryReporter{} + scanRep := &metrics.InMemoryReporter{} + tbl := New(nil, nil, "", nil, nil, WithMetricsReporter(tableRep)) + + // Scan-level WithReporter wins over the table's reporter. + scan := tbl.Scan(WithReporter(scanRep)) + assert.Same(t, scanRep, scan.Reporter()) + require.NotSame(t, tableRep, scan.Reporter()) + + // A nil WithReporter is ignored; the inherited reporter is kept. + scanNil := tbl.Scan(WithReporter(nil)) + assert.Same(t, tableRep, scanNil.Reporter()) +} + +// reporterStubCatalog is a minimal catalog whose LoadTable returns a table +// carrying the configured reporter (or the default nop reporter when nil). It +// lets the Refresh tests control exactly which reporter the "fresh" table +// arrives with. +type reporterStubCatalog struct { + reporter metrics.Reporter +} + +func (c *reporterStubCatalog) LoadTable(_ context.Context, ident Identifier) (*Table, error) { + opts := []Option{} + if c.reporter != nil { + opts = append(opts, WithMetricsReporter(c.reporter)) + } + + return New(ident, nil, "", + func(context.Context) (iceio.IO, error) { return iceio.LocalFS{}, nil }, c, opts...), nil +} + +func (c *reporterStubCatalog) CommitTable(context.Context, Identifier, []Requirement, []Update) (Metadata, string, error) { + return nil, "", nil +} + +// TestStagedTableInheritsReporter pins that Transaction.StagedTable() forwards +// the transaction table's reporter. StagedTable rebuilds the table via New(...), +// which would otherwise reset the reporter to the construction-time nop default. +func TestStagedTableInheritsReporter(t *testing.T) { + schema := iceberg.NewSchema(1, iceberg.NestedField{ + ID: 1, Name: "id", Type: iceberg.PrimitiveTypes.Int64, Required: true, + }) + meta, err := NewMetadata(schema, iceberg.UnpartitionedSpec, UnsortedSortOrder, + "mem://default/staged", iceberg.Properties{PropertyFormatVersion: "2"}) + require.NoError(t, err) + + rep := &metrics.InMemoryReporter{} + tbl := New(Identifier{"default", "staged"}, meta, "", + func(context.Context) (iceio.IO, error) { return iceio.NewMemFS(), nil }, nil, + WithMetricsReporter(rep)) + + staged, err := tbl.NewTransaction().StagedTable() + require.NoError(t, err) + assert.Same(t, rep, staged.MetricsReporter(), + "StagedTable must forward the transaction table's reporter") +} + +// on: a reporter injected via WithMetricsReporter must survive a Refresh even Review Comment: These two doc comments lost their leading function name — this one starts mid-sentence at "on:", and the one on `TestRefreshInheritsCatalogReporterWhenUnset` (line 150) opens lowercase at "the caller never overrode". Looks like the `// TestX ...` opener got clipped on both. Worth restoring the convention since these are the tests doing the load-bearing work here. -- 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]
