Core: Add metrics reporter for serializable table #7144

wangyinsheng · 2023-03-19T11:56:15Z

The metrics reporter will be lost when a table be sent to other nodes in a cluster through serializable table, This mr rebuild the metrics reporter when the table is deserialized

wangyinsheng · 2023-03-20T09:04:10Z

PTAL @nastra

nastra · 2023-03-20T11:39:59Z

core/src/main/java/org/apache/iceberg/SerializableTable.java

  }

+  @Override
+  public MetricsReporter metricsReporter() {


can we add a test that makes sure the metrics reporter is preserved with the right properties?

nastra · 2023-03-24T06:40:31Z

api/src/test/java/org/apache/iceberg/TestHelpers.java

    Assert.assertEquals("History must match", expected.history(), actual.history());
  }

+  public static void assertSerializedMetricsReporter(


no need to add this here, this can be just part of the test method itself

nastra · 2023-03-24T06:41:11Z

api/src/test/java/org/apache/iceberg/TestHelpers.java

+    Assert.assertEquals(
+        "metrics reporter from serializableTable should equals the one from origin table",
+        expected,
+        actual);


rather than testing actual equality on the objects, it should be enough to make sure that the properties are the same, hence the TestMetricsReporter wouldn't need to implement hasCode/equals

nastra · 2023-03-24T06:42:35Z

core/src/test/java/org/apache/iceberg/TestSerializableTable.java

+import org.junit.After;
+import org.junit.Rule;
+import org.junit.Test;
+import org.junit.rules.TemporaryFolder;


please take a look at https://iceberg.apache.org/contribute/#testing. Basically new test classes should be JUnit5 rather than JUnit4

nastra · 2023-03-24T06:43:15Z

core/src/test/java/org/apache/iceberg/TestTables.java

            throw new CommitFailedException("Injected failure");
          }
          Integer version = VERSIONS.get(tableName);
+          String metadataLocation =


why are these changes necessary?

the method lazyTable in SerializableTable will load metadata file, while the TestTable has not write metadata file

I still think those changes are not necessary. The test passed fine for me without having those changes

nastra · 2023-03-24T06:44:37Z

api/src/test/java/org/apache/iceberg/TestHelpers.java

+  public static void assertSerializedMetricsReporter(
+      MetricsReporter expected, MetricsReporter actual) {
+    Assert.assertNotNull("metrics reporter from serializableTable should not be null", actual);
+    Assert.assertTrue(


this can be simplified to Assertions.assertThat(actual).isNotNull().isInstanceOf(...) and for the properties: Assertions.assertThat(actual.properties).isEqualTo(...)

done, thanks

nastra · 2023-03-24T14:12:15Z

core/src/test/java/org/apache/iceberg/TestSerializableTable.java

+  public void testSerializableTableWithMetricsReporter()
+      throws IOException, ClassNotFoundException {
+    Map<String, String> properties = Maps.newHashMap();
+    properties.put(


nit: can be simplified to Map<String, String> properties = ImmutableMap.of(...)

nastra · 2023-03-24T14:13:07Z

core/src/test/java/org/apache/iceberg/TestSerializableTable.java

+
+  private static final SortOrder SORT_ORDER = SortOrder.builderFor(SCHEMA).asc("id").build();
+
+  @TempDir public File temp;


I think this doesn't have to be public to work

nastra · 2023-03-24T14:16:04Z

api/src/main/java/org/apache/iceberg/Table.java

+   */
+  default MetricsReporter metricsReporter() {
+    throw new UnsupportedOperationException(
+        "metricsReporter is not supported by " + getClass().getName());


Suggested change

"metricsReporter is not supported by " + getClass().getName());

"Accessing metrics reporter is not supported");

nastra · 2023-03-24T14:16:56Z

api/src/test/java/org/apache/iceberg/TestHelpers.java

    }
  }
+
+  public static class TestMetricsReporter implements MetricsReporter {


can we also move this to the respective test class? I don't think it's necessary to put this into TestHelpers

nastra · 2023-03-24T14:32:29Z

core/src/test/java/org/apache/iceberg/TestSerializableTable.java

+    properties.put(
+        CatalogProperties.METRICS_REPORTER_IMPL, TestHelpers.TestMetricsReporter.class.getName());
+    MetricsReporter reporter = CatalogUtil.loadMetricsReporter(properties);
+    Table table = TestTables.create(temp, "tbl_A", SCHEMA, SPEC, SORT_ORDER, 2, reporter);


I don't see where we're testing that the reporter itself has the correct properties.

Shouldn't this do

Map<String, String> reporterProperties = ImmutableMap.of("a", "1", "b", "2"); reporter.initialize(reporterProperties);

reporter.initialize(reporterProperties) is called in the method CatalogUtil.loadMetricsReporter(properties)

wangyinsheng · 2023-03-25T03:55:48Z

core/src/main/java/org/apache/iceberg/SerializableTable.java

+    if (lazyMetricsReporter == null) {
+      synchronized (this) {
+        if (lazyMetricsReporter == null) {
+          lazyMetricsReporter = CatalogUtil.loadMetricsReporter(this.metricsReporterProperties);


metrics reporter is configured by catalog properties, not table properties. I add the interface method MetricsReporter.properties() to return the properties which used to load and initialize this MetricsReporter, therefor the SerializableTable can use this properties to load and initialize MetricsReporter agagin

nastra · 2023-03-24T14:35:24Z

core/src/test/java/org/apache/iceberg/TestSerializableTable.java

+  @Test
+  public void testSerializableTableWithMetricsReporter()
+      throws IOException, ClassNotFoundException {
+    Map<String, String> properties = Maps.newHashMap();


those are the properties of the table, so you'd have to pass them to the table. The metrics reporter then has separate properties

metrics reporter is configured by catalog properties, not table properties. Here this properties is taken as properties of catalog

wangyinsheng · 2023-03-25T03:55:48Z

core/src/main/java/org/apache/iceberg/SerializableTable.java

+    if (lazyMetricsReporter == null) {
+      synchronized (this) {
+        if (lazyMetricsReporter == null) {
+          lazyMetricsReporter = CatalogUtil.loadMetricsReporter(this.metricsReporterProperties);


metrics reporter is configured by catalog properties, not table properties. I add the interface method MetricsReporter.properties() to return the properties which used to load and initialize this MetricsReporter, therefor the SerializableTable can use this properties to load and initialize MetricsReporter agagin

wangyinsheng · 2023-03-28T00:58:03Z

PTAL @nastra

nastra

I did take another look and I don't think we need the changes in TestTables, because the tests will pass without those.

wangyinsheng · 2023-03-28T14:07:50Z

I did take another look and I don't think we need the changes in TestTables, because the tests will pass without those.

you're right, it not necessary. I used to compare the metadata between origin table and serializable table, then it will be needed. I have removed the changes, PTAL @nastra

nastra

LGTM, thanks @wangyinsheng.

@aokolnychyi could you also take a look please?

wangyinsheng · 2023-03-30T00:45:32Z

PTAL @aokolnychyi

wangyinsheng · 2023-03-31T00:43:36Z

PTAL @aokolnychyi

wangyinsheng · 2023-04-03T13:47:09Z

could you please take a look this pr, it has last for weeks @aokolnychyi

wangyinsheng · 2023-04-04T10:21:18Z

@nastra Could you please approval the CI ? And is there someone else can review this pr?

wangyinsheng · 2023-04-10T08:28:00Z

@aokolnychyi @danielcweeks could you take a look please ? thanks

nastra · 2023-04-12T09:51:43Z

@wangyinsheng to fix CI failures I think you need to add the below code to TransactionTable:

    @Override
    public MetricsReporter metricsReporter() {
      return BaseTransaction.this.reporter;
    }

and then also this to BaseMetadataTable:

  @Override
  public MetricsReporter metricsReporter() {
    return table().metricsReporter();
  }

nastra · 2023-04-12T15:30:03Z

core/src/main/java/org/apache/iceberg/SerializableTable.java

+    this.metricsReporterProperties =
+        table instanceof BaseTable
+            ? SerializableMap.copyOf(table.metricsReporter().properties())
+            : null;


we should rather do #7144 (comment) than setting this to null here.

Sorry, I missed the comment when I try to fix the CI failures, I wll change it later.

nastra · 2023-04-13T11:48:15Z

@wangyinsheng I talked to @rdblue yesterday and we concluded that we don't want to carry over the catalog properties just for the metrics reporter. The better approach would be to make MetricsReporter extend Serializable. I opened wangyinsheng#1 against your branch to do that. Feel free to squash it into your changes

wangyinsheng · 2023-04-14T01:20:16Z

@nastra I am confused why should't carry over the catalog properties for metrics reporter? We already support to config metrics report through catalog properites. And metrics reporter may contains non-serializable object, KafakProducer for example. We need properties to recreate non-serializable object after table been deserialized

wangyinsheng · 2023-04-21T02:05:56Z

please take a look @nastra @rdblue @aokolnychyi @danielcweeks

wangyinsheng · 2023-04-24T09:17:57Z

please take a look @nastra @rdblue @aokolnychyi @danielcweeks

nastra · 2023-04-24T10:28:24Z

@wangyinsheng as mentioned previously, we still would like to make the MetricsReporter Serializable. For things that aren't Serializable in a metrics reporter, we would make them transient and then lazily initialize them

rdblue · 2023-06-04T18:37:19Z

@nastra, where are we with the fix for serialization here?

nastra · 2023-06-06T07:59:35Z

@nastra, where are we with the fix for serialization here?

@rdblue I haven't heard back from @wangyinsheng after my comment above , so I've opened #7370 a while ago for initial experimentation and then updated it to fix the serialization issue.

wangyinsheng · 2023-06-12T07:58:18Z

@nastra, where are we with the fix for serialization here?

@rdblue I haven't heard back from @wangyinsheng after my comment above , so I've opened #7370 a while ago for initial experimentation and then updated it to fix the serialization issue.

@nastra @rdblue Sorry，I am working on other things for a while. But I am still keep my opinion, I think the SerializableTable is the best place to handle the lazily initialization stuff for MetricsReporter, we should not just make MetricsReporter serializable, which will throw the lazily initialization stuff to any implemention class of MetricsReporter

github-actions · 2024-08-28T00:13:58Z

This pull request has been marked as stale due to 30 days of inactivity. It will be closed in 1 week if no further activity occurs. If you think that’s incorrect or this pull request requires a review, please simply write any comment. If closed, you can revive the PR at any time and @mention a reviewer or discuss it on the dev@iceberg.apache.org list. Thank you for your contributions.

github-actions · 2024-09-05T00:13:42Z

This pull request has been closed due to lack of activity. This is not a judgement on the merit of the PR in any way. It is just a way of keeping the PR queue manageable. If you think that is incorrect, or the pull request requires review, you can revive the PR at any time.

github-actions bot added API core labels Mar 19, 2023

wangyinsheng force-pushed the metrics_reporter_for_serializable_table branch 2 times, most recently from 882b279 to 6742851 Compare March 19, 2023 12:23

nastra reviewed Mar 20, 2023

View reviewed changes

wangyinsheng force-pushed the metrics_reporter_for_serializable_table branch from 6742851 to a08be15 Compare March 24, 2023 00:57

nastra requested changes Mar 24, 2023

View reviewed changes

wangyinsheng force-pushed the metrics_reporter_for_serializable_table branch from a08be15 to 69356e7 Compare March 24, 2023 13:56

nastra requested changes Mar 24, 2023

View reviewed changes

wangyinsheng force-pushed the metrics_reporter_for_serializable_table branch from 69356e7 to 9838834 Compare March 25, 2023 03:41

wangyinsheng commented Mar 25, 2023

View reviewed changes

nastra reviewed Mar 28, 2023

View reviewed changes

wangyinsheng force-pushed the metrics_reporter_for_serializable_table branch from 9838834 to 2ac204b Compare March 28, 2023 13:57

nastra approved these changes Mar 28, 2023

View reviewed changes

nastra requested review from aokolnychyi and danielcweeks April 4, 2023 10:32

wangyinsheng force-pushed the metrics_reporter_for_serializable_table branch 2 times, most recently from 08afaff to 4ea3cc0 Compare April 12, 2023 14:41

Core: Add metrics reporter for serializable table

f831280

wangyinsheng force-pushed the metrics_reporter_for_serializable_table branch from 4ea3cc0 to f831280 Compare April 12, 2023 15:03

nastra requested changes Apr 12, 2023

View reviewed changes

better way to fix CI failures

5ac30f0

wangyinsheng requested a review from nastra April 18, 2023 04:06

nastra mentioned this pull request Apr 18, 2023

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

Merged

nastra requested a review from rdblue April 18, 2023 14:42

github-actions bot added the stale label Aug 28, 2024

github-actions bot closed this Sep 5, 2024


		private static final SortOrder SORT_ORDER = SortOrder.builderFor(SCHEMA).asc("id").build();

		@TempDir public File temp;

	"metricsReporter is not supported by " + getClass().getName());
	"Accessing metrics reporter is not supported");

Core: Add metrics reporter for serializable table #7144

Core: Add metrics reporter for serializable table #7144

Uh oh!

Conversation

wangyinsheng commented Mar 19, 2023

Uh oh!

wangyinsheng commented Mar 20, 2023

Uh oh!

nastra Mar 20, 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wangyinsheng commented Mar 28, 2023

Uh oh!

nastra left a comment

Choose a reason for hiding this comment

Uh oh!

wangyinsheng commented Mar 28, 2023

Uh oh!

nastra left a comment

Choose a reason for hiding this comment

Uh oh!

wangyinsheng commented Mar 30, 2023

Uh oh!

wangyinsheng commented Mar 31, 2023

Uh oh!

wangyinsheng commented Apr 3, 2023

Uh oh!

nastra Mar 20, 2023 •

edited

Loading

nastra commented Apr 12, 2023 •

edited

Loading

wangyinsheng Apr 13, 2023 •

edited

Loading