wgtmac commented on code in PR #701:
URL: https://github.com/apache/iceberg-cpp/pull/701#discussion_r3499803057
##########
src/iceberg/catalog/rest/rest_catalog.cc:
##########
@@ -524,6 +544,21 @@ Result<std::shared_ptr<FileIO>> RestCatalog::TableFileIO(
return file_io_;
}
+std::shared_ptr<MetricsReporter> RestCatalog::MakeTableReporter(
+ const TableIdentifier& identifier) const {
+ auto enabled = config_.Get(RestCatalogProperties::kMetricsReportingEnabled);
+ if (StringUtils::ToLower(enabled) == "true" &&
+ supported_endpoints_.contains(Endpoint::ReportMetrics())) {
+ auto path = paths_->Metrics(identifier);
+ if (path.has_value()) {
+ auto rest_reporter =
+ std::make_shared<RestMetricsReporter>(client_, *path,
catalog_session_);
Review Comment:
This `RestMetricsReporter` still uses the catalog-level auth session even
after `MakeTableFromLoadResult` creates a table-scoped session for the table.
Java wires the REST metrics reporter with the table REST client, so metrics
POSTs use the same table-scoped credentials/headers as table operations. With a
REST catalog that returns table-specific credentials, scan/commit metrics can
be posted with the wrong auth and get silently dropped.
##########
src/iceberg/catalog/rest/rest_catalog.cc:
##########
@@ -483,6 +495,14 @@ RestCatalog::RestCatalog(RestCatalogProperties config,
std::shared_ptr<FileIO> f
snapshot_mode_(snapshot_mode),
default_context_(std::move(default_context)) {
ICEBERG_DCHECK(catalog_session_ != nullptr, "catalog_session must not be
null");
+ const auto& props = config_.configs();
+ auto it = props.find(std::string(kMetricsReporterImpl));
+ if (it != props.end() && !it->second.empty() &&
+ it->second != kMetricsReporterTypeNoop) {
+ if (auto r = MetricsReporters::Load(props); r.has_value()) {
Review Comment:
The REST catalog loads the custom metrics reporter twice: once in the
constructor here and again in `RestCatalog::Make` after construction. That
means a valid reporter factory is constructed and initialized twice, and the
first instance is immediately discarded. Reporter initialization can have side
effects or allocate resources; Java loads and initializes the configured
reporter once. The constructor load should be removed and the `Make` path
should be the single source of truth.
--
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]
