tanmayrauth commented on PR #1655:
URL: https://github.com/apache/iceberg-go/pull/1655#issuecomment-5210353884
Thanks for the review @zeroshade. I've pushed a commit that addresses
every item. Summary:
- Enablement is now client-only. Reporting is gated on
reportMetricsEnabled(r.reporterProps), which reads the properties captured
before /v1/config merges in, exactly as reporter selection already does. Server
defaults, overrides,
and table-response properties can no longer turn it on.
- Reports have a finite timeout. Each POST runs under
context.WithTimeout(dispatcherCtx, timeout) (default 10s, configurable via
rest-metrics-reporting-timeout-ms), covering auth + full request/response.
- Dispatch is bounded. Per-report goroutines are gone; reports now go
through a catalog-owned worker pool (4 workers, 128-slot queue). A full queue
drops the report and logs it.
- Close now does its job. Catalog.Close cancels in-flight reports and
drains the workers, bounded by the timeout so shutdown can't hang.
- Canonical key. Renamed to rest-metrics-reporting-enabled; the dotted
spelling is kept as a backward-compatible alias.
- Tests. Added coverage for finite timeout, bounded concurrency + drop,
Close cancelling in-flight and dropping queued reports, full /v1/{prefix}/...
path construction with escaping, and the enablement precedence matrix (server
defaults/overrides can't enable; endpoint must be advertised).
--
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]
