Replacing Apache commons-lang3 object serializer with Kryo serializer #583

ovj · 2019-02-22T20:52:45Z

Fixes this issue #580. (more discussions are present here. #495).
Switching over to kryo serializer to allow serialization of all spark supported objects including non-java serializable objects.

ovj · 2019-02-22T20:55:01Z

@n3nash @bvaradar @vinothchandar PTAL.

vinothchandar

Top level question is : can kryo serialize everything java serialization can? This looks self contained and kryo would be more efficient.. But are there tradeoffs here, we are making

hoodie-common/pom.xml

vinothchandar · 2019-03-01T18:02:46Z

hoodie-common/src/main/java/com/uber/hoodie/common/util/SerializationUtils.java

 */
 public class SerializationUtils {
+
+  // Caching kryo serializer to avoid creating kryo instance for every serde operation
+  private static final AtomicReference<KryoSerializerInstance> serializerRef =


is this threadsafe? in case someone runs with more than 1 core? Consider using ThreadLocal?

It is thread safe. See here. http://tutorials.jenkov.com/java-util-concurrent/atomicreference.html

With ThreadLocal we will end up creating more than one copy of it.

AtomicReference just provides the ability atomically manipulate the reference itself. I am talking about two threads using the same reference.. i.e is the KryoSerializerInstance itself threadsafe?

ThreadLocal we will end up creating more than one copy of it.

that's the idea. Have one per write task.

We are guaranteeing that single thread is using KryoSerializerInstance by getAndSet() in borrowKryoSerializerInstance(). But I think it makes sense to use ThreadLocal instead. Let me change it.

hoodie-common/src/main/java/com/uber/hoodie/common/util/SerializationUtils.java

vinothchandar · 2019-03-01T18:10:46Z

hoodie-common/src/main/java/com/uber/hoodie/common/util/SerializationUtils.java

-      throw new IllegalArgumentException("The byte[] must not be null");
+  // Returns cached kryo serializer instance if available otherwise creates a new copy of it.
+  private static KryoSerializerInstance borrowKryoSerializerInstance() {
+    final KryoSerializerInstance kryoSerializerInstance = SerializationUtils.serializerRef


if we make it ThreadLocal, how about just a static allocation without any atomic references/need for concurrency?

See above comment about ThreadLocal. Let me know what you think. Will change it according to what we decide.

commented above.

changing it to ThreadLocal

vinothchandar · 2019-03-01T18:12:14Z

hoodie-common/src/main/java/com/uber/hoodie/common/util/SerializationUtils.java

+    KryoSerializerInstance() {
+      EmptyScalaKryoInstantiator kryoInstantiator = new EmptyScalaKryoInstantiator();
+      kryo = kryoInstantiator.newKryo();
+      baos = new ByteArrayOutputStream(1024);


pull 1024 into a constant and consider if this needs to be larger by default.. Looks like the simplest payload will be bigger?

Sg. Will change it to 1M. 1M should be sufficient for most cases.

n3nash · 2019-03-02T00:17:06Z

hoodie-common/src/main/java/com/uber/hoodie/common/util/SerializationUtils.java

+      kryo.reset();
+      baos.reset();
+      Output output = new Output(baos);
+      ObjectWrapper wrapper = new ObjectWrapper(obj);


For every serialization, a new Wrapper object is created ? I see that you need a class to read/serialize from kryo but anyway to avoid this ?

Unfortunately not. I also wanted to avoid it but could not figure out a way without this.

This is also needed for handling null values. By using Wrapper class we can actually serde "null" values.

How is this done in spark? does it allocate a wrapper object as well?

Checked in spark. They are using different Kryo's api which handles null objects. Changing it to that.

ovj · 2019-03-02T01:39:53Z

Top level question is : can kryo serialize everything java serialization can? This looks self contained and kryo would be more efficient.. But are there tradeoffs here, we are making

Kryo can serialize all objects. Infact it also allows us to specify serializers for individual classes if we want to avoid using Kryo's default FieldSerializer.

Only catch here is that in case of Java serialization it uses writeObject() & readObject() apis if you have overridden one. Kryo will not use those apis for serde. But if we really want kryo to use them then we can always register this class to use instead JavaSerializer() using "spark.kryo.registrator" spark config.

Details of kryo serialization performance are here. https://github.com/EsotericSoftware/kryo

ovj · 2019-03-02T04:37:36Z

@vinothchandar @n3nash - Addressed all review comments. Can you PTAL again?

vinothchandar

HoodieRecordPayload still extends Serializable ..

public interface HoodieRecordPayload<T extends HoodieRecordPayload> extends Serializable {

is this necessary? Can we add javadocs or in site docs in that class to provide guidance on how to author new payloads that work with kryo?

hoodie-common/pom.xml

vinothchandar · 2019-03-11T17:40:33Z

hoodie-common/src/main/java/com/uber/hoodie/common/util/SerializationUtils.java

 */
 public class SerializationUtils {
+
+  // Caching kryo serializer to avoid creating kryo instance for every serde operation
+  private static final AtomicReference<KryoSerializerInstance> serializerRef =


AtomicReference just provides the ability atomically manipulate the reference itself. I am talking about two threads using the same reference.. i.e is the KryoSerializerInstance itself threadsafe?

ThreadLocal we will end up creating more than one copy of it.

that's the idea. Have one per write task.

vinothchandar · 2019-03-11T17:40:55Z

hoodie-common/src/main/java/com/uber/hoodie/common/util/SerializationUtils.java

-      throw new IllegalArgumentException("The byte[] must not be null");
+  // Returns cached kryo serializer instance if available otherwise creates a new copy of it.
+  private static KryoSerializerInstance borrowKryoSerializerInstance() {
+    final KryoSerializerInstance kryoSerializerInstance = SerializationUtils.serializerRef


commented above.

hoodie-common/src/main/java/com/uber/hoodie/common/util/SerializationUtils.java

vinothchandar · 2019-03-11T17:42:23Z

hoodie-common/src/main/java/com/uber/hoodie/common/util/SerializationUtils.java

+      kryo.reset();
+      baos.reset();
+      Output output = new Output(baos);
+      ObjectWrapper wrapper = new ObjectWrapper(obj);


How is this done in spark? does it allocate a wrapper object as well?

ovj

addressed all the comments @bvaradar / @vinothchandar PTAL.

ovj · 2019-03-12T02:25:59Z

hoodie-common/src/main/java/com/uber/hoodie/common/util/SerializationUtils.java

+      kryo.reset();
+      baos.reset();
+      Output output = new Output(baos);
+      ObjectWrapper wrapper = new ObjectWrapper(obj);


Checked in spark. They are using different Kryo's api which handles null objects. Changing it to that.

ovj · 2019-03-12T02:28:10Z

hoodie-common/src/main/java/com/uber/hoodie/common/util/SerializationUtils.java

-      throw new IllegalArgumentException("The byte[] must not be null");
+  // Returns cached kryo serializer instance if available otherwise creates a new copy of it.
+  private static KryoSerializerInstance borrowKryoSerializerInstance() {
+    final KryoSerializerInstance kryoSerializerInstance = SerializationUtils.serializerRef


changing it to ThreadLocal

hoodie-common/pom.xml

bvaradar

@ovj @vinothchandar : Package changes looks good

vinothchandar

One final comment on buffer size..

Also since this changes the query side bundles, should we run the integ-test once to ensure its all good once? @bvaradar to chime in

vinothchandar · 2019-03-18T17:56:55Z

hoodie-common/src/main/java/com/uber/hoodie/common/util/SerializationUtils.java

+  }
+
+  private static class KryoSerializerInstance implements Serializable {
+    public static final int KRYO_SERIALIZER_BUFFER_SIZE = 1048576;


@ovj qq.. does this buffer grow if large objects come along? is this just an intial size? if so, can you rename the variable as well. if not, do we need to expose a parameter for this?

Yes this is an initial buffer size. if we try to write larger object then it will auto grow.

renamed it.

Thanks.. Ready to merge, as long as integ-test passes..

bvaradar · 2019-03-18T18:37:36Z

@ovj : can you run mvn integration-test and check if they succeed.

ovj · 2019-03-18T19:58:16Z

Thanks @bvaradar - Ran this "mvn clean integration-test". It passes.

…mmit metadata (apache#583)

vinothchandar requested changes Mar 1, 2019

View reviewed changes

n3nash reviewed Mar 2, 2019

View reviewed changes

ovj force-pushed the issues_333 branch from 7a4b87b to 29b2511 Compare March 2, 2019 04:35

vinothchandar requested changes Mar 11, 2019

View reviewed changes

ovj force-pushed the issues_333 branch from 29b2511 to 13c537d Compare March 12, 2019 21:36

ovj commented Mar 12, 2019

View reviewed changes

bvaradar reviewed Mar 13, 2019

View reviewed changes

vinothchandar approved these changes Mar 18, 2019

View reviewed changes

Replacing Apache commons-lang3 object serializer with Kryo serializer

c35a6ef

ovj force-pushed the issues_333 branch from 13c537d to c35a6ef Compare March 18, 2019 18:29

vinothchandar merged commit a6c45fe into apache:master Mar 18, 2019

vinishjail97 pushed a commit to vinishjail97/hudi that referenced this pull request Mar 13, 2024

[HUDI-7491] Fixing handling null values of extra metadata in clean co…

0f66970

…mmit metadata (apache#583)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Replacing Apache commons-lang3 object serializer with Kryo serializer #583

Replacing Apache commons-lang3 object serializer with Kryo serializer #583

ovj commented Feb 22, 2019 •

edited

Loading

ovj commented Feb 22, 2019

vinothchandar left a comment

vinothchandar Mar 1, 2019

ovj Mar 1, 2019

vinothchandar Mar 11, 2019

ovj Mar 12, 2019

vinothchandar Mar 1, 2019

ovj Mar 2, 2019

vinothchandar Mar 11, 2019

ovj Mar 12, 2019

ovj Mar 12, 2019

vinothchandar Mar 1, 2019

ovj Mar 2, 2019

n3nash Mar 2, 2019

ovj Mar 2, 2019

vinothchandar Mar 11, 2019

ovj Mar 12, 2019

ovj commented Mar 2, 2019

ovj commented Mar 2, 2019

vinothchandar left a comment

vinothchandar Mar 11, 2019

vinothchandar Mar 11, 2019

vinothchandar Mar 11, 2019

ovj left a comment

ovj Mar 12, 2019

ovj Mar 12, 2019

bvaradar left a comment

vinothchandar left a comment

vinothchandar Mar 18, 2019

ovj Mar 18, 2019

ovj Mar 18, 2019

vinothchandar Mar 18, 2019

bvaradar commented Mar 18, 2019

ovj commented Mar 18, 2019

Replacing Apache commons-lang3 object serializer with Kryo serializer #583

Replacing Apache commons-lang3 object serializer with Kryo serializer #583

Conversation

ovj commented Feb 22, 2019 • edited Loading

ovj commented Feb 22, 2019

vinothchandar left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ovj commented Mar 2, 2019

ovj commented Mar 2, 2019

vinothchandar left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ovj left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bvaradar left a comment

Choose a reason for hiding this comment

vinothchandar left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bvaradar commented Mar 18, 2019

ovj commented Mar 18, 2019

ovj commented Feb 22, 2019 •

edited

Loading