Core: Make metrics reporter serializable (alternative impl) #8032

nastra · 2023-07-10T13:26:34Z

This is an alternative implementation to #7370. This PR avoids breaking the API and rather makes the HTTPClient serializable, where the underlying CloseableHttpClient and ObjectMapper are both loaded dynamically

aws/src/main/java/org/apache/iceberg/aws/s3/signer/S3V4RestSignerClient.java

nastra · 2023-07-10T13:30:26Z

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


-    this.httpClient = clientBuilder.build();
+    try {
+      return DynMethods.builder("mapper").hiddenImpl(impl).buildStaticChecked().invoke();


we're relying on a static mapper() method, which isn't ideal. Currently we only have 2 classes that follow this pattern, namely RESTObjectMapper and S3ObjectMapper

the other alternative to lazily load the object mapper I can think of is that we could make all Ser/De classes in RESTSerializers and S3ObjectMapper implement Serializable

oops, while making all Ser/De classes implement Serializable would work for normal Java ser/de, it doesn't for Kryo without kryo.register(ObjectMapper.class, new JavaSerializer()); as it fails with

Unable to find class: sun.reflect.GeneratedConstructorAccessor48 Serialization trace: delegate (sun.reflect.DelegatingConstructorAccessorImpl) constructorAccessor (java.lang.reflect.Constructor) _constructor (com.fasterxml.jackson.databind.introspect.AnnotatedConstructor) _defaultCreator (com.fasterxml.jackson.databind.deser.std.StdValueInstantiator) _valueInstantiator (com.fasterxml.jackson.databind.deser.BeanDeserializer) _rootDeserializers (com.fasterxml.jackson.databind.ObjectMapper) mapper (org.apache.iceberg.rest.HTTPClient) client (org.apache.iceberg.rest.RESTMetricsReporter) reporters (org.apache.iceberg.metrics.MetricsReporters$CompositeMetricsReporter) metricsReporter (org.apache.iceberg.SerializableTable) com.esotericsoftware.kryo.KryoException: Unable to find class: sun.reflect.GeneratedConstructorAccessor48 Serialization trace: delegate (sun.reflect.DelegatingConstructorAccessorImpl) constructorAccessor (java.lang.reflect.Constructor) _constructor (com.fasterxml.jackson.databind.introspect.AnnotatedConstructor) _defaultCreator (com.fasterxml.jackson.databind.deser.std.StdValueInstantiator) _valueInstantiator (com.fasterxml.jackson.databind.deser.BeanDeserializer) _rootDeserializers (com.fasterxml.jackson.databind.ObjectMapper) mapper (org.apache.iceberg.rest.HTTPClient) client (org.apache.iceberg.rest.RESTMetricsReporter) reporters (org.apache.iceberg.metrics.MetricsReporters$CompositeMetricsReporter) metricsReporter (org.apache.iceberg.SerializableTable) at com.esotericsoftware.kryo.util.DefaultClassResolver.readName(DefaultClassResolver.java:160) at com.esotericsoftware.kryo.util.DefaultClassResolver.readClass(DefaultClassResolver.java:133) at com.esotericsoftware.kryo.Kryo.readClass(Kryo.java:693) at com.esotericsoftware.kryo.serializers.ObjectField.read(ObjectField.java:118) at com.esotericsoftware.kryo.serializers.FieldSerializer.read(FieldSerializer.java:543) at com.esotericsoftware.kryo.Kryo.readObject(Kryo.java:731)

core/src/main/java/org/apache/iceberg/io/ResolvingFileIO.java

rdblue · 2023-07-28T17:15:38Z

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

+    if (null == mapper) {
+      synchronized (this) {
+        if (null == mapper) {
+          mapper = loadObjectMapperDynamically(properties.getOrDefault(OBJECT_MAPPER_IMPL, null));


I don't think we should load this dynamically. Can we just add the signer request and response objects to the same object mapper? I'm sure there are other options.

I've moved away from dynamically loading this and instead went with #8032 (comment)

rdblue · 2023-07-28T17:18:03Z

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

+
+  Object writeReplace() {
+    // fetch the latest headers from the AuthSession and carry them over in a separate supplier so
+    // that AuthSession doesn't have to be Serializable


Why change AuthSession to use a SerializableMap if it doesn't need to be serializable?

we need to change this for Kryo ser/de, otherwise it will fail with the below error. This is because we're passing a Lambda with the headers from AuthSession and Kryo ser/des everything in the scope of that Lambda

headers (org.apache.iceberg.rest.RESTMetricsReporter) reporters (org.apache.iceberg.metrics.MetricsReporters$CompositeMetricsReporter) metricsReporter (org.apache.iceberg.SerializableTable) com.esotericsoftware.kryo.KryoException: java.lang.RuntimeException: Could not serialize lambda Serialization trace: headers (org.apache.iceberg.rest.RESTMetricsReporter) reporters (org.apache.iceberg.metrics.MetricsReporters$CompositeMetricsReporter) metricsReporter (org.apache.iceberg.SerializableTable) at com.esotericsoftware.kryo.serializers.ObjectField.read(ObjectField.java:144) at com.esotericsoftware.kryo.serializers.FieldSerializer.read(FieldSerializer.java:543) at com.esotericsoftware.kryo.Kryo.readClassAndObject(Kryo.java:813) at com.esotericsoftware.kryo.serializers.CollectionSerializer.read(CollectionSerializer.java:134) at com.esotericsoftware.kryo.serializers.CollectionSerializer.read(CollectionSerializer.java:40) at com.esotericsoftware.kryo.Kryo.readObject(Kryo.java:731) at com.esotericsoftware.kryo.serializers.ObjectField.read(ObjectField.java:125) at com.esotericsoftware.kryo.serializers.FieldSerializer.read(FieldSerializer.java:543) at com.esotericsoftware.kryo.Kryo.readObject(Kryo.java:731) at com.esotericsoftware.kryo.serializers.ObjectField.read(ObjectField.java:125) at com.esotericsoftware.kryo.serializers.FieldSerializer.read(FieldSerializer.java:543) at com.esotericsoftware.kryo.Kryo.readClassAndObject(Kryo.java:813) at org.apache.iceberg.TestHelpers$KryoHelpers.roundTripSerialize(TestHelpers.java:294) at org.apache.iceberg.rest.TestRESTCatalog.testScanReportingOnSerializableTable(TestRESTCatalog.java:2033) .... Caused by: java.lang.RuntimeException: Could not serialize lambda at com.esotericsoftware.kryo.serializers.ClosureSerializer.read(ClosureSerializer.java:78) at com.esotericsoftware.kryo.Kryo.readObject(Kryo.java:731) at com.esotericsoftware.kryo.serializers.ObjectField.read(ObjectField.java:125) ... 97 more Caused by: com.esotericsoftware.kryo.KryoException: java.lang.UnsupportedOperationException Serialization trace: headers (org.apache.iceberg.rest.auth.OAuth2Util$AuthSession) capturedArgs (java.lang.invoke.SerializedLambda) at com.esotericsoftware.kryo.serializers.ObjectField.read(ObjectField.java:144) at com.esotericsoftware.kryo.serializers.FieldSerializer.read(FieldSerializer.java:543) at com.esotericsoftware.kryo.Kryo.readObject(Kryo.java:731) at com.esotericsoftware.kryo.serializers.DefaultArraySerializers$ObjectArraySerializer.read(DefaultArraySerializers.java:391) at com.esotericsoftware.kryo.serializers.DefaultArraySerializers$ObjectArraySerializer.read(DefaultArraySerializers.java:302) at com.esotericsoftware.kryo.Kryo.readObject(Kryo.java:731) at com.esotericsoftware.kryo.serializers.ObjectField.read(ObjectField.java:125) at com.esotericsoftware.kryo.serializers.FieldSerializer.read(FieldSerializer.java:543) at com.esotericsoftware.kryo.Kryo.readObject(Kryo.java:709) at com.esotericsoftware.kryo.serializers.ClosureSerializer.read(ClosureSerializer.java:75) ... 99 more Caused by: java.lang.UnsupportedOperationException at org.apache.iceberg.relocated.com.google.common.collect.ImmutableMap.put(ImmutableMap.java:781) at com.esotericsoftware.kryo.serializers.MapSerializer.read(MapSerializer.java:162) at com.esotericsoftware.kryo.serializers.MapSerializer.read(MapSerializer.java:39) at com.esotericsoftware.kryo.Kryo.readObject(Kryo.java:731) at com.esotericsoftware.kryo.serializers.ObjectField.read(ObjectField.java:125)

For standard Java ser/de we're relying on the writeReplace() method. I've updated the comment to make it clear that this is only for standard Java ser/de

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

rdblue · 2023-07-28T17:22:35Z

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

+  @Test
+  public void testScanReportingOnSerializableTable() throws IOException, ClassNotFoundException {
+    Table table = catalog().buildTable(TABLE, SCHEMA).create();
+    table


Do we not send scan metrics if the table is empty?

correct, we don't send anything if the table doesn't have any files to do scan planning for

rdblue · 2023-07-28T17:22:59Z

Thanks, @nastra! This looks like it's a lot closer than the alternative.

github-actions · 2024-09-06T00:14:38Z

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-10-09T00:14:54Z

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.

manuzhang · 2024-11-08T05:34:11Z

@nastra are you still working on this? I might need a public BaseTable#metricsReporter for #11340 as well.

nastra · 2024-11-08T10:55:33Z

@manuzhang the PR is still valid but I didn't have time to re-visit it and fix the merge conflicts. I'll do that next week or so and hopefully @rdblue can review it then

github-actions · 2025-01-16T00:15:12Z

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 · 2025-01-30T00:14:41Z

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 AWS labels Jul 10, 2023

nastra mentioned this pull request Jul 10, 2023

Core: Make metrics reporter serializable #7370

Closed

nastra requested a review from rdblue July 10, 2023 13:28

nastra commented Jul 10, 2023

View reviewed changes

aws/src/main/java/org/apache/iceberg/aws/s3/signer/S3V4RestSignerClient.java Outdated Show resolved Hide resolved

nastra commented Jul 10, 2023

View reviewed changes

nastra force-pushed the serializable-metrics-reporter-serializable-lazy-http-client branch from 40ff541 to 54ea53e Compare July 10, 2023 13:35

rdblue reviewed Jul 28, 2023

View reviewed changes

core/src/main/java/org/apache/iceberg/io/ResolvingFileIO.java Outdated Show resolved Hide resolved

rdblue reviewed Jul 28, 2023

View reviewed changes

core/src/test/java/org/apache/iceberg/rest/TestRESTCatalog.java Show resolved Hide resolved

rdblue reviewed Jul 28, 2023

View reviewed changes

nastra force-pushed the serializable-metrics-reporter-serializable-lazy-http-client branch 3 times, most recently from ecb2f65 to 9f01740 Compare August 8, 2023 15:11

github-actions bot removed the AWS label Aug 8, 2023

nastra mentioned this pull request Aug 9, 2023

Core: Re-apply Hadoop conf if it's null after Kryo ser/de in ResolvingFileIO #8270

Merged

nastra force-pushed the serializable-metrics-reporter-serializable-lazy-http-client branch 2 times, most recently from 28b02bd to a55a0a3 Compare August 10, 2023 16:07

Core: Make metrics reporter serializable

74e0263

nastra force-pushed the serializable-metrics-reporter-serializable-lazy-http-client branch from a55a0a3 to 74e0263 Compare September 13, 2023 15:55

nastra requested a review from rdblue September 13, 2023 15:56

github-actions bot added the stale label Sep 6, 2024

nastra removed the stale label Sep 6, 2024

github-actions bot added the stale label Oct 9, 2024

nastra removed the stale label Oct 9, 2024

nastra mentioned this pull request Nov 28, 2024

CommitReport not reported through custom MetricsReporter #11664

Closed

3 tasks

github-actions bot added the stale label Jan 16, 2025

github-actions bot closed this Jan 30, 2025

Core: Make metrics reporter serializable (alternative impl) #8032

Core: Make metrics reporter serializable (alternative impl) #8032

Uh oh!

Conversation

nastra commented Jul 10, 2023

Uh oh!

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!

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!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rdblue commented Jul 28, 2023

Uh oh!

github-actions bot commented Sep 6, 2024

Uh oh!

github-actions bot commented Oct 9, 2024

Uh oh!

manuzhang commented Nov 8, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nastra commented Nov 8, 2024

Uh oh!

github-actions bot commented Jan 16, 2025

Uh oh!

github-actions bot commented Jan 30, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

manuzhang commented Nov 8, 2024 •

edited

Loading