Core: Extract REST metrics reporter into its own class #7339

nastra · 2023-04-13T12:49:18Z

depends on #7337

core/src/main/java/org/apache/iceberg/rest/RESTSessionCatalog.java

core/src/main/java/org/apache/iceberg/rest/RESTMetricsReporter.java

nastra · 2023-04-13T13:02:07Z

core/src/main/java/org/apache/iceberg/rest/RESTMetricsReporter.java

+    }
+  }
+
+  private TableIdentifier identifierWithoutCatalog(String tableNameWithCatalog) {


The table name in reports always carries the catalog name as the first part. I might have missed it, but I haven't found anything in the codebase that would give us the TableIdentifier from the fully-qualified table name (that also carries the catalog's name). Should this functionality maybe live somewhere else?

It seems like a bad idea to parse this. Can we add the identifier to the ScanReport instead? Then we could use the identifier that was loaded. Maybe we should add it to BaseTable as well, like the Python implementation does.

I completely agree here. It would be great if the actual table would carry a TableIdentifier, then we could just use that. I'll open a separate issue to improve that.

Can we add the identifier to the ScanReport instead?

for a ScanReport we take the table name from the table directly:

for a CommitReport we take it indirectly via CreateSnapshotEvent

So I think the first step would be to add TableIdentifier to Table and then use that TableIdentifier for ScanReport/CommitReport.

I've opened #7419

nastra · 2023-04-18T09:57:24Z

core/src/main/java/org/apache/iceberg/rest/RESTMetricsReporter.java

+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+public class RESTMetricsReporter implements MetricsReporter {


making RESTMetricsReporter fully Serializable will be handled in a separate PR as it currently also requires #7144

rdblue · 2023-04-22T00:32:28Z

core/src/main/java/org/apache/iceberg/rest/RESTMetricsReporter.java

+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+class RESTMetricsReporter implements MetricsReporter {


Serializable? Or is this just to produce a separate one as a minimal refactor?

for easier reviewing, the goal of this PR was to move REST metrics reporting into its own class before making it fully serializable (as that involves slightly more changes). I have a WIP PR (currently outdated) for this that also includes the required changes from #7144

core/src/main/java/org/apache/iceberg/rest/RESTSessionCatalog.java

core/src/main/java/org/apache/iceberg/rest/RESTMetricsReporter.java

rdblue · 2023-04-22T00:35:35Z

core/src/test/java/org/apache/iceberg/rest/TestRESTCatalog.java

            // different method calls
            if (!"v1/oauth/tokens".equals(path)) {
-              if ("v1/config".equals(path)) {
+              if ("v1/config".equals(path) || path.endsWith("/metrics")) {


Why was this needed?

this is because now we only instantiate a single metrics reporter and use the headers from the catalog's AuthSession.
In case that is a concern, we could instantiate a new reporter wherever we currently pass in the reporter and then use the headers from session::headers.

In fact that approach might be better as we wouldn't need to derive the TableIdentifier from the metrics report.

nastra · 2023-04-24T10:18:06Z

core/src/main/java/org/apache/iceberg/rest/RESTSessionCatalog.java

-      }
-    } catch (Exception e) {
-      LOG.warn("Failed to report metrics to REST endpoint for table {}", tableIdentifier, e);
+  private MetricsReporter metricsReporter(


rather than wrapping the combined reporter in a lambda for the purpose of reporting, we just return the combined reporter here.

github-actions bot added the core label Apr 13, 2023

nastra commented Apr 13, 2023

View reviewed changes

core/src/main/java/org/apache/iceberg/rest/RESTSessionCatalog.java Outdated Show resolved Hide resolved

nastra commented Apr 13, 2023

View reviewed changes

core/src/main/java/org/apache/iceberg/rest/RESTMetricsReporter.java Outdated Show resolved Hide resolved

nastra commented Apr 13, 2023

View reviewed changes

core/src/main/java/org/apache/iceberg/rest/RESTMetricsReporter.java Outdated Show resolved Hide resolved

nastra commented Apr 13, 2023

View reviewed changes

nastra force-pushed the extract-rest-metrics-reporter branch 2 times, most recently from 61558e2 to 35aa55a Compare April 13, 2023 13:53

nastra closed this Apr 13, 2023

nastra reopened this Apr 13, 2023

nastra force-pushed the extract-rest-metrics-reporter branch from 57284c5 to 75ecf6f Compare April 18, 2023 09:55

nastra commented Apr 18, 2023

View reviewed changes

This was referenced Apr 18, 2023

Core: Make metrics reporter serializable #7370

Closed

Core: Introduce CompositeMetricsReporter #7337

Merged

nastra force-pushed the extract-rest-metrics-reporter branch from 75ecf6f to c48fd88 Compare April 20, 2023 08:12

github-actions bot added the API label Apr 20, 2023

nastra force-pushed the extract-rest-metrics-reporter branch 2 times, most recently from 8cae49f to 335206a Compare April 21, 2023 06:27

nastra requested a review from rdblue April 21, 2023 15:36

nastra force-pushed the extract-rest-metrics-reporter branch from 335206a to 72148d7 Compare April 21, 2023 15:55

github-actions bot removed the API label Apr 21, 2023

rdblue reviewed Apr 22, 2023

View reviewed changes

core/src/main/java/org/apache/iceberg/rest/RESTSessionCatalog.java Outdated Show resolved Hide resolved

rdblue reviewed Apr 22, 2023

View reviewed changes

core/src/main/java/org/apache/iceberg/rest/RESTMetricsReporter.java Outdated Show resolved Hide resolved

rdblue reviewed Apr 22, 2023

View reviewed changes

nastra force-pushed the extract-rest-metrics-reporter branch 2 times, most recently from 12b5f79 to a29d3b2 Compare April 24, 2023 10:13

Core: Extract REST metrics reporter into its own class

e047145

nastra force-pushed the extract-rest-metrics-reporter branch from a29d3b2 to e047145 Compare April 24, 2023 10:14

nastra commented Apr 24, 2023

View reviewed changes

rdblue approved these changes Apr 25, 2023

View reviewed changes

rdblue merged commit 8e73813 into apache:master Apr 25, 2023

nastra deleted the extract-rest-metrics-reporter branch April 25, 2023 18:00

manisin pushed a commit to Snowflake-Labs/iceberg that referenced this pull request May 9, 2023

Core: Extract REST metrics reporter into its own class (apache#7339)

3061095

zhongyujiang pushed a commit to zhongyujiang/iceberg that referenced this pull request Apr 16, 2025

Core: Extract REST metrics reporter into its own class (apache#7339)

83b1e24

Core: Extract REST metrics reporter into its own class #7339

Core: Extract REST metrics reporter into its own class #7339

Uh oh!

Conversation

nastra commented Apr 13, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

nastra Apr 13, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rdblue Apr 22, 2023

Choose a reason for hiding this comment

Uh oh!

nastra Apr 24, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nastra Apr 24, 2023

Choose a reason for hiding this comment

Uh oh!

nastra Apr 18, 2023

Choose a reason for hiding this comment

Uh oh!

rdblue Apr 22, 2023

Choose a reason for hiding this comment

Uh oh!

nastra Apr 24, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

rdblue Apr 22, 2023

Choose a reason for hiding this comment

Uh oh!

nastra Apr 24, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nastra Apr 24, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

nastra commented Apr 13, 2023 •

edited

Loading

nastra Apr 13, 2023 •

edited

Loading

nastra Apr 24, 2023 •

edited

Loading

nastra Apr 24, 2023 •

edited

Loading

nastra Apr 24, 2023 •

edited

Loading

nastra Apr 24, 2023 •

edited

Loading