tanmayrauth commented on PR #1430:
URL: https://github.com/apache/iceberg-go/pull/1430#issuecomment-4972421635
Both in, and I made the deliberate call on Close() you asked for.
Commit-path guard — fixed, and you were exactly right about the mechanism:
MetricsReporter() is never nil, so carrying it through doCommit via
WithMetricsReporter(...) flipped reporterSet true on every commit and froze a
catalog-defaulted table after its first commit. I replaced it with a private
withReporterState(t.reporter, t.reporterSet) that copies both fields verbatim,
applied at both doCommit and StagedTable, so a defaulted table stays defaulted
across a commit. TestCommitPreservesDefaultedReporterInheritance pins
create+Commit+Refresh — I confirmed it fails against the old line (freezes to
nop) and passes now.
Close() / per-catalog lifetime — I'd floated deferring this last round; on
reflection you're right that it's the moment to decide, and since adding it
later is the breaking change, I pinned it now instead of taking the default:
- Reporter now embeds io.Closer (Java's Closeable shape). Built-ins close
as no-ops; the composite closes its children with panic isolation + error
joining. The interface surface is settled, so no external implementer eats a
break later.
- I also did the per-catalog half rather than leave it half-solved: added
metrics.CachedReporter, which resolves the reporter once and caches it, and
wired every catalog (rest, glue, sql, hadoop, hive) to hold one plus a Close()
that releases it — hive's existing Close now closes the reporter too. So the
first stateful HTTP reporter is built once per catalog and has an owner that
closes it, matching Java.
The one thing I deliberately left out is Close() on the catalog.Catalog
interface itself — that's a much broader breaking change to a
widely-implemented public interface, so it feels like its own decision rather
than something to fold in here. Catalogs expose Close() as concrete methods for
now (reachable via an io.Closer assertion, same as hive already did). Happy to
take interface-level catalog Close to a follow-up if you'd want it — but the
reporter's own lifetime and cleanup are handled in this PR.
--
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]
