[HUDI-5579] Fixing Kryo registration to be properly wired into Spark sessions #7702

alexeykudinkin · 2023-01-18T23:24:21Z

Change Logs

Due to RFC-46 the profile of the data being serialized by Hudi had changed considerably: previously we're mostly passing around Avro payloads, while now we hold our own internal HoodieRecord implementations.

When classes are not explicitly registered w/ Kryo, it would have to serialize class fully qualified name (FQN) as id every time an object is serialized, which carries a lot of unnecessary overhead.

To work this around in #7026 added HoodieSparkKryoRegistrar registering some of the commonly serialized Hudi classes. However, during rebasing/merging of the RFC-46 feature branch this changes have been partially reverted and this PR takes a stab at reinstating these.

On top of that we had to revisit our current approach to bundling and shading Kryo universally for all bundles. Instead

For engines providing Kryo (Spark, Flink) we're not bundling it at all
For other bundles still requiring it we bundle and shade it (same way we do it today)

Impact

This will improve performance of ser/de during shuffles, since no FQNs will need to be serialized.

Risk level (write none, low medium or high below)

Low: our bundle validation and testing should uncover issues w/ the packaging if any

Documentation Update

Documentation update is required now specifying that --conf spark.kryo.registrator=org.apache.spark.HoodieSparkKryoRegistrar property will be mandatory

Contributor's checklist

Read through contributor's guide
Change Logs and Impact were stated clearly
Adequate tests were added if applicable
CI passed

danny0405 · 2023-01-19T04:52:17Z

hudi-client/hudi-spark-client/src/main/scala/org/apache/spark/HoodieSparkKryoRegistrar.scala

+  // NOTE: We're copying definition of the config introduced in Spark 3.0
+  //       (to stay compatible w/ Spark 2.4)
+  private val KRYO_USER_REGISTRATORS = "spark.kryo.registrator"
+


Guess we can make it public so that there is no need to hard code the option key spark.kryo.registrator everywhere.

We actually won't be able to use it everywhere, so i rather stuck w/ the Spark option for consistency (which is the way we handle every other option as well)

danny0405 · 2023-01-19T04:54:01Z

hudi-client/hudi-spark-client/src/main/scala/org/apache/spark/HoodieSparkKryoRegistrar.scala

  def register(conf: SparkConf): SparkConf = {
-    conf.registerKryoClasses(new HoodieSparkKryoProvider().registerClasses())
+    conf.set(KRYO_USER_REGISTRATORS, Seq(classOf[HoodieSparkKryoRegistrar].getName).mkString(","))
  }


Does .mkString(",") make sense here?

We need to convert it to a string, so i kept it generic so that we can drop in one more class. Not strictly necessary though

danny0405 · 2023-01-19T04:55:22Z

...-client/hudi-spark-client/src/test/java/org/apache/hudi/testutils/HoodieClientTestUtils.java

+        .setMaster("local[4]")
+        .set("spark.serializer", "org.apache.spark.serializer.KryoSerializer")
+        .set("spark.kryo.registrator", "org.apache.spark.HoodieSparkKryoRegistrar")
+        .set("spark.sql.extensions", "org.apache.spark.sql.hudi.HoodieSparkSessionExtension")


Can we also move these common options into a tool method?

This is exactly the method you're referring to (used in tests)

danny0405 · 2023-01-19T04:58:28Z

hudi-common/src/main/java/org/apache/hudi/common/util/HoodieCommonKryoRegistrar.java

        HoodieRecordGlobalLocation.class
-    };
+    })
+        .forEachOrdered(kryo::register);


A stateless function (function that does not take any side effect) is always a better choice especially for tool method, personally I prefer the old way we handle this.

Agree in principle, but here we actually aligning it w/ an interface of KryoRegistrator

danny0405 · 2023-01-19T05:01:31Z

...-datasource/hudi-spark/src/test/scala/org/apache/spark/sql/hudi/HoodieSparkSqlTestBase.scala

-    .config("spark.serializer", "org.apache.spark.serializer.KryoSerializer")
-    .config("hoodie.insert.shuffle.parallelism", "4")
-    .config("hoodie.upsert.shuffle.parallelism", "4")
-    .config("hoodie.delete.shuffle.parallelism", "4")


Not sure whether we can remove these parallelism options.

These are not removed -- they are replaced w/ options set in getSparkConfForTest

danny0405 · 2023-01-19T05:07:35Z

pom.xml

-            <relocation>
-              <pattern>com.esotericsoftware.kryo.</pattern>
-              <shadedPattern>org.apache.hudi.com.esotericsoftware.kryo.</shadedPattern>
-            </relocation>


What is the purpose to move the common bundle dependencies to each bundle pom files?

We actually move it just to bundles that will have the Kryo included (Spark and Flink won't have Kryo included)

xushiyan

lgtm

xushiyan · 2023-01-20T00:01:32Z

--conf spark.kryo.registrator=org.apache.spark.HoodieSparkKryoRegistrar

but this is a very broad usability change. we should have brought this up for highlighting earlier.

alexeykudinkin · 2023-01-20T00:41:02Z

but this is a very broad usability change. we should have brought this up for highlighting earlier.

Agreed, not ideal, but unavoidable unfortunately -- w/o we'd be passing around ~20-30% more dead-weight data. And in some cases it would actually lead to failures as well.

alexeykudinkin · 2023-01-20T07:21:59Z

@hudi-bot run azure

- For engines shipping Kryo (Spark, Flink) we'll not be adding it at all (and no shading therefore) - For other engines we'll be shipping shaded Kryo (as we do today)

…link's own "kryo"

hudi-bot · 2023-01-21T10:02:04Z

CI report:

ae1bcf3 Azure: CANCELED

Bot commands

@hudi-bot supports the following commands:

@hudi-bot run azure re-run the last Azure build

alexeykudinkin · 2023-01-21T17:15:37Z

CI is green:

https://dev.azure.com/apache-hudi-ci-org/apache-hudi-ci/_build/results?buildId=14524&view=results

…o Spark sessions (apache#7702)" This reverts commit a70355f.

…sessions (apache#7702) ### Change Logs Due to RFC-46 the profile of the data being serialized by Hudi had changed considerably: previously we're mostly passing around Avro payloads, while now we hold our own internal `HoodieRecord` implementations. When classes are not explicitly registered w/ Kryo, it would have to serialize class fully qualified name (FQN) as id every time an object is serialized, which carries a lot of [unnecessary overhead](https://github.com/apache/hudi/pull/7026/files#diff-81f9b48f7f7e71b46ea8764c7d63e310c871895d03640ae93c81b09f38306acb). To work this around in apache#7026 added `HoodieSparkKryoRegistrar` registering some of the commonly serialized Hudi classes. However, during rebasing/merging of the RFC-46 feature branch this changes have been partially reverted and this PR takes a stab at reinstating these. On top of that we had to revisit our current approach to bundling and shading Kryo universally for all bundles. Instead - For engines providing Kryo (Spark, Flink) we're not bundling it at all - For other bundles still requiring it we bundle and shade it (same way we do it today)

alexeykudinkin requested review from danny0405 and xushiyan January 18, 2023 23:24

alexeykudinkin added the priority:blocker Production down; release blocker label Jan 18, 2023

alexeykudinkin assigned xushiyan Jan 18, 2023

alexeykudinkin changed the title ~~Fixing Kryo registration to be properly wired into Spark sessions~~ [HUDI-5579] Fixing Kryo registration to be properly wired into Spark sessions Jan 18, 2023

alexeykudinkin force-pushed the ak/kryo-shd-fix branch 3 times, most recently from 70fcf7c to 11cceab Compare January 19, 2023 00:50

alexeykudinkin added the engine:spark Spark integration label Jan 19, 2023

alexeykudinkin mentioned this pull request Jan 19, 2023

[HUDI-4937] Fix HoodieTable injecting non-reusable HoodieBackedTableMetadata aggressively flushing MT readers #6815

Closed

4 tasks

danny0405 reviewed Jan 19, 2023

View reviewed changes

alexeykudinkin force-pushed the ak/kryo-shd-fix branch from e96bbfc to 61e6215 Compare January 19, 2023 17:23

xushiyan approved these changes Jan 19, 2023

View reviewed changes

apache deleted a comment from hudi-bot Jan 20, 2023

alexeykudinkin force-pushed the ak/kryo-shd-fix branch from 1b075e2 to 8218fde Compare January 20, 2023 16:42

Alexey Kudinkin added 11 commits January 20, 2023 17:58

Restored HoodieSparkKryoRegistrar to extend KryoRegistrar

3a40483

Register SerializableConfiguration w/ Kryo

129dc6d

Fixing tests missing Kryo registrar config

2e55fd5

Wire in Kryo Registrar into more tests

2aa127a

Fixing compilation

c4ea857

Excluded Kryo as universal dependency, instead

93ae5c2

- For engines shipping Kryo (Spark, Flink) we'll not be adding it at all (and no shading therefore) - For other engines we'll be shipping shaded Kryo (as we do today)

Updated all usages to reference HoodieSparkKryoRegistrar

0035012

Fixing missed HoodieSparkSessionExtension config

c3939bc

Fixed Hudi rules initialization to properly specify the target ctor

eb429f3

Fixing tests

cd63a6d

Reinstating existing parallelism level

de70d52

Alexey Kudinkin added 5 commits January 20, 2023 17:58

Fixed Flink modules to bring in "kryo-shaded" instead of relying on F…

b39029b

…link's own "kryo"

Added missing "kryo-shaded" to hudi-timeline-service

807a2b6

Added Kryo to hudi-integ-test-bundle

f474c11

Missing Kryo deps

0e0a0fe

Added missing kryo deps

c372fe7

alexeykudinkin force-pushed the ak/kryo-shd-fix branch from ffe3e3b to c372fe7 Compare January 21, 2023 01:58

alexeykudinkin merged commit a70355f into apache:master Jan 21, 2023

lokeshj1703 added a commit to lokeshj1703/hudi that referenced this pull request Jan 23, 2023

Revert "[HUDI-5579] Fixing Kryo registration to be properly wired int…

34904ec

…o Spark sessions (apache#7702)" This reverts commit a70355f.

xushiyan mentioned this pull request Jan 30, 2023

[HUDI-5637] Add Kryo for hive sync #7781

Merged

4 tasks

[HUDI-5579] Fixing Kryo registration to be properly wired into Spark sessions #7702

[HUDI-5579] Fixing Kryo registration to be properly wired into Spark sessions #7702

Uh oh!

Conversation

alexeykudinkin commented Jan 18, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Change Logs

Impact

Risk level (write none, low medium or high below)

Documentation Update

Contributor's checklist

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!

xushiyan left a comment

Choose a reason for hiding this comment

Uh oh!

xushiyan commented Jan 20, 2023

Uh oh!

alexeykudinkin commented Jan 20, 2023

Uh oh!

alexeykudinkin commented Jan 20, 2023

Uh oh!

hudi-bot commented Jan 21, 2023

CI report:

Uh oh!

alexeykudinkin commented Jan 21, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

alexeykudinkin commented Jan 18, 2023 •

edited

Loading