search

gaob13 / kryo

Automatically exported from code.google.com/p/kryo
BSD 3-Clause "New" or "Revised" License
0 stars 0 forks source link

Factory pattern for instantiation of serializers #121

Closed GoogleCodeExporter closed 8 years ago

GoogleCodeExporter commented 8 years ago 
In Kryo, it is not possible to add a custom serializer as a default serializer 
via Kryo.setDefaultSerializer if the Serializer requires a custom constructor. 
This can be avoided by implementing a factory pattern where constructors can be 
called manually.

I stepped onto this when implementing Spring bean aware serializers for Kyro in 
the scope of my own project which I suggested to the kryo-serializers project 
on Git Hub: https://github.com/magro/kryo-serializers/pull/11

In the discussion to this pull request, I described a use case scenario where 
such semantics give meaning. That way, these serializers can be implemented for 
a general audience without needing to use a custom implementation of Kryo.

Patch file is attached. If you are interested in the patch, let me know such 
that I can add some Java doc.

Original issue reported on code.google.com by rafael....@gmail.com on 19 Jul 2013 at 2:09

Attachments:

GoogleCodeExporter commented 8 years ago 
PS: my IDE tricked me. I'll submit a patch without white space changes on 
monday.

Original comment by rafael....@gmail.com on 19 Jul 2013 at 3:48

GoogleCodeExporter commented 8 years ago 
This time without my company's source code style sheet.

Original comment by rafael....@gmail.com on 22 Jul 2013 at 7:15

Attachments:

GoogleCodeExporter commented 8 years ago 
Hi Rafael, 

Thanks for submitting a patch. The second version is definitely nicer and 
smaller ;-) There are still a few indentation problems, I think. But aside from 
that, the code is simple enough to follow. 

@Nate: What do you think about this proposal? Do you think we should include 
something like this?

Original comment by romixlev on 23 Jul 2013 at 1:38

GoogleCodeExporter commented 8 years ago 
Hey guys, I was wondering if there was any progress on that matter? I am 
currently using a forked version of Kryo, but I would love to return to the 
official distribution in the long run. Thanks for keeping me updated!

Original comment by rafael....@gmail.com on 4 Aug 2013 at 8:26

GoogleCodeExporter commented 8 years ago 
@Nate: Any comments from your side about it?

-Leo

Original comment by romixlev on 5 Aug 2013 at 11:00

GoogleCodeExporter commented 8 years ago 
Is it really so bad to subclass Kryo and override newDefaultSerializer?

Original comment by nathan.s...@gmail.com on 5 Aug 2013 at 12:18

GoogleCodeExporter commented 8 years ago 
Well, relying on an overriden newDefaultSerializer method breaks the expected 
behavior of the Kryo class. Of course, both ways do the job, but if you want to 
offer a custom serializer to be used by others which might use their own Kryo 
configurations, you either have to tell them explicitly to additionally 
overriding the method of their Kryo instances or otherwise have runtime 
exceptions thrown. 

I consider this a hack and this is why I suggested the change. See this example 
of a Spring aware serializer https://github.com/magro/kryo-serializers/pull/11 
for why I think this matters.

Original comment by rafael....@gmail.com on 5 Aug 2013 at 12:39

GoogleCodeExporter commented 8 years ago 
Ok. I think it makes sense. I'm fine with the patch being applied. Would you 
like to do the honors, Leo? I'm absolutely buried in work. :(

PseudoSerializerFactory is an interesting class name. :)

Original comment by nathan.s...@gmail.com on 22 Aug 2013 at 7:26

GoogleCodeExporter commented 8 years ago 
OK. I'll try to apply the patch tomorrow, run all our unit tests, and if 
everything is fine I'll commit it.

Original comment by romixlev on 22 Aug 2013 at 7:37

GoogleCodeExporter commented 8 years ago 
OK. I merged the patch into trunk.

@Raphael: If you could provide some Javadocs based on the trunk version 
containing your changes, it would be nice.

Original comment by romixlev on 23 Aug 2013 at 2:12

GoogleCodeExporter commented 8 years ago 
Ok, I will send some Javadocs the next days.

Original comment by rafael....@gmail.com on 23 Aug 2013 at 3:34

GoogleCodeExporter commented 8 years ago 
And here it is.

Original comment by rafael....@gmail.com on 25 Aug 2013 at 1:53

Attachments:

GoogleCodeExporter commented 8 years ago 
Thanks, Rafael. I applied your Javadoc patch. Should we close this issue now?

Original comment by romixlev on 26 Aug 2013 at 6:25

GoogleCodeExporter commented 8 years ago 
From my side, there is nothing left to to. Thanks for the quick fix!

Original comment by rafael....@gmail.com on 26 Aug 2013 at 6:38

GoogleCodeExporter commented 8 years ago

Original comment by martin.grotzke on 30 Sep 2013 at 9:38