Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Changes to support better separation between applying global and type serialization #7

Merged
merged 3 commits into from
Oct 6, 2016

Conversation

wneild
Copy link

@wneild wneild commented Oct 3, 2016

Changes include:

  • The addition of a global serializer which can be configured with a custom UserSerializer for adding many custom Kryo serializers
  • Redesigned typed serializer to only take a single optional custom Kryo serializer
  • Method for registering customized global serialization

…l custom serialization configuration against specific type serialization
@jerrinot
Copy link
Owner

jerrinot commented Oct 4, 2016

@wneild: many thanks for your contribution. There are some integration tests failing, you can run them locally by calling mvn clean test -Pit

Some more thoughts: I think the separation between Global / Type-specific serializers is useful when you are creating a custom subclass. However I quite like to have a single out-of-the-box implementation - it makes the default user experience smoother - just a single class to deal with. Smooth user experience is one of the core features of this project.

Here is a proposal:

  1. keep a single out-of-the-box Serializer - it can be registered as a global serializer or for a specific type. Also make this class final so I cannot be sub-classesed
  2. Have 2 abstract classes for extensions: AbstractGlobalUserSerializier and AbstractTypeSpecificUserSerializler (or something like this)

What do you think?

@jerrinot jerrinot added this to the 0.7 milestone Oct 4, 2016
… added Global/Type-specific UserSerializer abstract implementations
@wneild
Copy link
Author

wneild commented Oct 4, 2016

@jerrinot thanks for the feedback! Good suggestion, I've fixed the tests I missed and made those changes.

The only thing I'm not sure on is the constructor argument for AbstractGlobalUserSerializer, I don't think the UserSerializer is an appropriate argument. I was thinking the UserSerializationBuilder might be a good fit for now, any thoughts?

import java.io.InputStream;
import java.io.OutputStream;

public abstract class AbstractSerializer<T> implements StreamSerializer<T>, HazelcastInstanceAware {
Copy link
Owner

@jerrinot jerrinot Oct 5, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this probably should be package private, not public. We do not want users to create own subclasses of this class.

import info.jerrinot.subzero.internal.PropertyUserSerializer;
import info.jerrinot.subzero.internal.strategy.TypedKryoStrategy;

public abstract class AbstractTypeSpecificUserSerializer<T> extends AbstractSerializer<T> {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

JavaDoc is needed here. with a description when and how-to extend this class

* @param serializerClazz Class of global serializer implementation to use
* @return Hazelcast configuration.
*/
public static <T> Config useAsGlobalSerializer(Config config, Class<? extends AbstractGlobalUserSerializer> serializerClazz) {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would not add this method. Methods in the SubZero class are for user-convenience. For a quick-start if the default SubZero is good enough for you. If you are implementing own serializer then you do not need the convenience.

I expect majority of users won't/shouldn't need to create custom subclasses. The extra methods here could be confusing for new users.

Copy link
Author

@wneild wneild Oct 5, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The way I see it, SubZero is all about providing convenience, so I'm not convinced about not having some convenience method to add custom serialization configuration. The alternative is forcing the user to write out what's already available internally to SubZero class when registering the custom global serializer to hazelcast, which seems a waste.

I couldn't comment on how widely used custom serialization is (probably about 10% of Kryo users going by github stars https://github.com/magro/kryo-serializers) but I will remove the methods if you're not convinced.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok, makes sense. let's keep it in.


import info.jerrinot.subzero.internal.strategy.GlobalKryoStrategy;

public abstract class AbstractGlobalUserSerializer<T> extends AbstractSerializer<T> {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

JavaDoc is missing. I think users will need to extend this classes if and only if

  1. they need to register custom Kryo serializers
  2. the out-of-the-box way to register custom serializers (=properties for now) is not sufficient.

Is this correct? Do you see any other user-case?

@jerrinot
Copy link
Owner

jerrinot commented Oct 5, 2016

@wneild: thanks for the updated PR. It looks good! I made some minor comments, please let me know what do you think about them.

Also +1 to UserSerializationBuilder Please go ahead and rename it.

…y of AbstractSerializer to package private
@jerrinot jerrinot merged commit f887d3f into jerrinot:master Oct 6, 2016
@jerrinot
Copy link
Owner

jerrinot commented Oct 6, 2016

good work, many thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants