gaborkaszab commented on code in PR #6404:
URL: https://github.com/apache/iceberg/pull/6404#discussion_r1045678055
##########
core/src/main/java/org/apache/iceberg/rest/RESTSessionCatalog.java:
##########
@@ -304,7 +306,9 @@ public Table loadTable(SessionContext context,
TableIdentifier identifier) {
new BaseTable(
ops,
fullTableName(loadedIdent),
- report -> reportMetrics(tableIdentifier, report,
session::headers));
Review Comment:
Just distantly related and only for my own information: With the current
implementation MetricsReporter could be configured when we start the Catalog
and then all the tables within this catalog would use the same MetricsReporter.
I'm wondering if it would make sense to make this setting more flexible, like
being able to set this even after the Catalog is up and running, and
additionally have the ability to use different MetricsReporter implementation
for different tables.
I don't have any specific use-cases in mind, one could be when let's say
querying a table becomes pretty slow at some point and we want to switch to
another MetricsReporter that could help us analysing the scan metrics to spot
the issue, and then switch back to LoggingMetricsReporter once the issue is
solved. (Assuming that in the future we gonna have way more scan metrics that
could be used for debugging such issues)
This is not meant to be covered in this PR just thinking out loud if it
makes sense.
##########
core/src/main/java/org/apache/iceberg/rest/RESTSessionCatalog.java:
##########
@@ -177,7 +177,9 @@ public void initialize(String name, Map<String, String>
unresolved) {
this.io =
CatalogUtil.loadFileIO(
ioImpl != null ? ioImpl : ResolvingFileIO.class.getName(),
mergedProps, conf);
- this.reporter = new LoggingMetricsReporter();
+ String metricsReporterImpl =
mergedProps.get(CatalogProperties.METRICS_REPORTER_IMPL);
Review Comment:
Would it make sense to add a test where you configure a custom
MetricsReporter through this new catalog property?
--
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]
