-
Notifications
You must be signed in to change notification settings - Fork 304
GIRAPH-1185 #69
GIRAPH-1185 #69
Conversation
| * | ||
| * @param <T> Object type | ||
| */ | ||
| public class KryoSimpleWrapper<T> implements Writable { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's the difference between this and KryoWritableWrapper, when should we use one and when the other?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have added more documentation now. The usage of KryoSimpleWrapper is similar to KryoWritabeWrapper. KryoSimpleWrapper can be used if the object being serialized is not recursive or nested, because it uses a version of kryo object that doesn't track references, hence significantly faster.
| * This serialization does not support recursive/nested structures | ||
| * to provide faster serialization. | ||
| */ | ||
| public abstract class KryoSimpleWritable implements KryoIgnoreWritable { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be great to switch to Java 8, to be able to keep this an interface :-)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree :-)
| // non-kryo serialization. | ||
| /** Reusable Input object */ | ||
| private final InputChunked input = new InputChunked(4096); | ||
| private InputChunked input; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we separate two Kryo implementations in two classes since they have a lot of differences?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed, we can refactor the existing code to have a better code readability. I have added a TODO for that.
| * no recursive/nested objects should be serialized. | ||
| * @return Hadoop kryo which doesn't track objects. | ||
| */ | ||
| public static HadoopKryo getNontrackingKryo() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How will someone using kryo know which implementation to use? Can we add some guidance around that?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added more documentation.
majakabiljo
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Discussed offline, @yukselakinci will in a separate diff refactor HadoopKryo.
closes apache#69
closes apache#69
Second PR for serialization. There is one more.