tanmayrauth commented on PR #1430:
URL: https://github.com/apache/iceberg-go/pull/1430#issuecomment-4960676514
@laskoviymishka Thanks for the thorough round-2 pass, everything's in.
For the Refresh guard, I switched to a reporterSet bool on Table that
WithMetricsReporter sets when r != nil, so Refresh now guards on if
!t.reporterSet instead of the NopReporter type assertion; that stops an
explicit WithMetricsReporter(metrics.NopReporter{}) opt-out (and Combine(nil,
nil)) from being conflated with "unset", drops the type dependency entirely,
and is pinned by a new TestRefreshKeepsExplicitNopOptOut where an explicit
NopReporter{} refreshed against a catalog handing back a live reporter stays
NopReporter{} — the case that failed silently before.
On the table-lifecycle paths, you were right that Transaction.StagedTable()
rebuilt via New(...) without forwarding the reporter and reset it to the nop
default, so I now pass WithMetricsReporter(t.tbl.MetricsReporter()) through
(covered by TestStagedTableInheritsReporter), which forwards whatever the
transaction's table carried since MetricsReporter() is never nil — and if you
or @zeroshade know of other New(...) rebuild sites on those paths, point me at
them and I'll sweep them in the same pass.
On Reporter.Close() and per-catalog lifetime I'd like to defer rather than
pull it into this PR, but as a conscious decision: all three built-ins
(NopReporter, LoggingReporter, InMemoryReporter) are stateless so Close() would
be a no-op we'd be committing to on the interface today, and matching Java's
Closeable properly also means moving to resolve-once-per-catalog with
deterministic shutdown, which is a larger change than this PR's scope — so
since adding Close() error later is a breaking change, I'll open a dedicated
follow-up to decide Close() plus per-catalog lifetime together before we call
the metrics API stable, though if you'd rather pin Close() now (no-op on the
built-ins) to avoid the break, wdyt?
Finally, the three non-blocking nits are also in: WithMetricsReporter(nil)
returns a shared noopTableOption mirroring WithReporter, the Hive metadata-read
error now uses %w, and the FromProperties godoc notes the nop default is an
intentional divergence from Java's LoggingMetricsReporter.
--
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]
