[BEAM-6021] Registered more internal classes for kryo serialization by mareksimunek · Pull Request #6998 · apache/beam

mareksimunek · 2018-11-09T11:58:19Z

Registred more internal classes for kryo serialization

Follow this checklist to help us incorporate your contribution quickly and easily:

Format the pull request title like [BEAM-XXX] Fixes bug in ApproximateQuantiles, where you replace BEAM-XXX with the appropriate JIRA issue, if applicable. This will automatically link the pull request to the issue.
If this contribution is large, please file an Apache Individual Contributor License Agreement.

It will help us expedite review of your Pull Request if you tag someone (e.g. @username) to look at it.

Post-Commit Tests Status (on master branch)

Lang	Apex	Dataflow	Flink	Gearpump	Samza	Spark
Go	---	---	---	---	---	---
Java
Python	---			---	---	---

mareksimunek · 2018-11-09T12:17:18Z

Run Spark ValidatesRunner

dmvk

This seems like a potentially great performance improvement. 👍 Did you notice any measurable performance improvement after switching from java serialization?

...src/main/java/org/apache/beam/runners/spark/stateful/SparkGroupAlsoByWindowViaWindowSet.java

runners/spark/src/main/java/org/apache/beam/runners/spark/translation/SparkContextFactory.java

lukecwik · 2018-11-09T18:03:22Z

There have been issues in the past about using Kryo as the serializer, this a discussion about it just over a year ago: https://lists.apache.org/thread.html/a36d68b568377f8064463b7fc374e8304d59a26412360050333bb2aa@%3Cdev.beam.apache.org%3E

It may not be an issue anymore though but wanted to provide context as to why this may have not worked in the past.

mareksimunek · 2018-11-16T10:23:22Z

As I investigate problems which Lukasz mentioned still last so I only added more classes to register so if you set KryoSerializer you have registered some of internals used objects by default.

mareksimunek · 2018-11-16T10:59:34Z

@iemejia pls code review

aviemzur

M2C: We made sure that everywhere that we need to serialize data we didn’t let Spark do it with its serializer, which defaults to Java unless configured otherwise by Spark's spark.serializer configuration. Spark users often configure this to be Kryo instead of Java, so we made sure to encode the data using Beam’s encoders, then passed Byte[] so when Spark serializes data before it is transmitted to other machines, it uses its serializer (Kryo/Java) to serialize a Byte[] which is always serializable by the serializer (Whether Java or Kryo).

If other classes, which are not the user's data are serialized by Kryo, and we're sure that they are always serializable by Kryo, that's fine.

I'm not sure this change actually makes the classes registered to be serialized by Kryo. Do we have tests to show this? IIRC this is controlled by the spark.serializer configuration.

mareksimunek · 2018-11-21T16:31:21Z

@aviemzur Hi, you are influenced by this change only if you use conf.set("spark.serializer", "org.apache.spark.serializer.KryoSerializer");
Added more internal classes which are used and test to show that is used KryoSerializer.

mareksimunek · 2018-11-22T12:40:34Z

Run Java PreCommit

mareksimunek · 2018-11-28T14:19:34Z

Run Java PreCommit

iemejia

Let some comments. Please ping me to re review and sorry for the delay.

iemejia · 2018-12-10T11:03:39Z

...spark/src/test/java/org/apache/beam/runners/spark/coders/BeamSparkRunnerRegistratorTest.java

+    conf.setAppName("test");
+    conf.set("spark.serializer", "org.apache.spark.serializer.KryoSerializer");
+    // register immutable collections serializers because the SDK uses them.
+    conf.set("spark.kryo.registrator", BeamSparkRunnerRegistrator.class.getName());


There seem to be some initialization error because the test seems not to be testing the BeamSparkRunnerRegistrator. Probably a good idea to assert that if calling MicrobatchSource it really uses StatelessJavaSerializer.

There was no easy way to hook if register of BeamSparkRunnerRegistrator is called so I created wrapper around regitrator, simple pipeline and created desired test if MicrobatchSource is registred

iemejia · 2018-12-10T11:05:13Z

...spark/src/test/java/org/apache/beam/runners/spark/coders/BeamSparkRunnerRegistratorTest.java

+import scala.reflect.ClassTag$;
+
+/** Testing of beam registrar. */
+public class BeamSparkRunnerRegistratorTest {


Probably worth to add an extra test that asserts that the Registrator is not called by default.

mareksimunek · 2018-12-18T14:05:46Z

Rebased and fixed test and added new one testing default behavior.
@iemejia pls review :)

…n using kryo's BeamSparkRunnerRegistrator

iemejia · 2019-01-08T10:33:42Z

Thanks a lot @mareksimunek and sorry for the extra long time to review. Already merged manually because I did some final touches, so closing it here.

dmvk reviewed Nov 9, 2018

View reviewed changes

...src/main/java/org/apache/beam/runners/spark/stateful/SparkGroupAlsoByWindowViaWindowSet.java Show resolved Hide resolved

runners/spark/src/main/java/org/apache/beam/runners/spark/translation/SparkContextFactory.java Outdated Show resolved Hide resolved

mareksimunek force-pushed the simunek/internalKryoSerializer branch 3 times, most recently from 5c6222f to 59e4a18 Compare November 13, 2018 08:50

mareksimunek changed the title ~~[BEAM-6021] Use KryoSerializer instead of JavaSerializer~~ [BEAM-6021] Registred more internal classes for kryo serialization Nov 16, 2018

aviemzur self-requested a review November 18, 2018 08:37

aviemzur reviewed Nov 18, 2018

View reviewed changes

mareksimunek force-pushed the simunek/internalKryoSerializer branch from 59e4a18 to 875d716 Compare November 21, 2018 16:25

mareksimunek force-pushed the simunek/internalKryoSerializer branch 2 times, most recently from 25d0332 to 51fec74 Compare November 26, 2018 10:55

iemejia changed the title ~~[BEAM-6021] Registred more internal classes for kryo serialization~~ [BEAM-6021] Registered more internal classes for kryo serialization Dec 10, 2018

iemejia requested changes Dec 10, 2018

View reviewed changes

mareksimunek added 2 commits December 18, 2018 10:49

[BEAM-6021] register used classes in Spark for Kryo

2abfa20

[BEAM-6021] fixed test and added new testing default behavior

ff1c700

mareksimunek force-pushed the simunek/internalKryoSerializer branch from 51fec74 to ff1c700 Compare December 18, 2018 14:02

iemejia added a commit that referenced this pull request Jan 8, 2019

Merge pull request #6998: [BEAM-6021] Serialize more core classes whe…

e38f0b2

…n using kryo's BeamSparkRunnerRegistrator

iemejia closed this Jan 8, 2019

iemejia mentioned this pull request Jan 8, 2019

[BEAM-5775] Spark: implement a custom class to lazily encode values for persistence. #6714

Closed

2 tasks

Conversation

mareksimunek commented Nov 9, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Registred more internal classes for kryo serialization

Post-Commit Tests Status (on master branch)

Uh oh!

mareksimunek commented Nov 9, 2018

Uh oh!

dmvk left a comment • 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!

lukecwik commented Nov 9, 2018

Uh oh!

mareksimunek commented Nov 16, 2018

Uh oh!

mareksimunek commented Nov 16, 2018

Uh oh!

aviemzur left a comment

Choose a reason for hiding this comment

Uh oh!

mareksimunek commented Nov 21, 2018

Uh oh!

mareksimunek commented Nov 22, 2018

Uh oh!

mareksimunek commented Nov 28, 2018

Uh oh!

iemejia left a comment

Choose a reason for hiding this comment

Uh oh!

iemejia Dec 10, 2018

Choose a reason for hiding this comment

Uh oh!

mareksimunek Dec 18, 2018

Choose a reason for hiding this comment

Uh oh!

iemejia Dec 10, 2018

Choose a reason for hiding this comment

Uh oh!

mareksimunek Dec 18, 2018

Choose a reason for hiding this comment

Uh oh!

mareksimunek commented Dec 18, 2018

Uh oh!

iemejia commented Jan 8, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

mareksimunek commented Nov 9, 2018 •

edited

Loading

dmvk left a comment •

edited

Loading