Handle Composite's name serialization properly

[vaidhy]

When a composite is serialized and then deserialized, the components of composite are typed as ByteBuffer. This patch enables the use of right kind of serializers for composite.

This patch contains the following:

1. When a composite is constructed or components are added, the serializer list is initialized properly.
2. CompositeSerializer stores the list of serializers, so that the same serializers can be applied while deserializing.
3. CompositeSerializer now includes methods to set the serializers, so that query results can be constructed properly.
4. Remove the singleton CompositeSerializer since it would contain instance specific data (serializer list)
5. Test cases to test the new CompositeSerializer

[ezoerner]

It looks like this issue was closed without the fix being merged into the product? I ran into this same problem in release 1.1-2, the symptom being a NullPointerException. The CompositeSerializer defaults to ByteType which is not in the type-to-comparator mapping in AbstractComposite resulting in an NPE on deserialization. I coded my own fix which looks a lot like vaidhy's. So I'm wondering why this issue was closed.

[zznate]

The merge above is in master currently. Can you compare your change with the following and send me a patch:
https://github.com/hector-client/hector/blob/master/core/src/main/java/me/prettyprint/hector/api/beans/AbstractComposite.java

If something is amiss, we'll get it sorted out. Thanks.

[ezoerner]

I'm not referring to the change in AbstractComposite here which is also in release 1.1-2. I don't believe that that change actually addresses the original issue (#503) unless I am missing something.

I am referring to vaidhy's pull request which is a change to CompositeSerializer that takes a list of serializers at construction time. I am a bit confused actually, because I don't see how deserialization of a Composite could possibly work without the serializers being properly set for each component.

I looked into the NPE I was getting, and this does not occur in master due to the use of a different version of the google guava library.

Nate's suggestion to use a factory to create a proper CompositeSerializer is fine, but it doesn't fix the problem with the singleton CompositeSerializer which is broken for deserialization.

I have just submitted a pull request that contains a modified CompositeSerializer and also has two unit tests that demonstrate the problem it fixes. The test that uses the singleton CompositeSerializer fails, and the test that constructs a CompositeSerializer with the proper serializers succeeds.

[ezoerner]

It looks to me that what happened here is that the wrong issue was closed. Nate's commit should have closed issue #500 instead of #503. (Although #500 was closed by Lanny, so that needs to be sorted out)

#503 should be reopened.

[ezoerner]

I'm still getting used to how GitHub works. Seems pull requests and issues are essentially the same thing (?), so apparently my pull request, #585, sort of does already re-open this issue.

[vaidhy]

Hi Eric,

I decided to build my own CompositeSerializer due to the exact same
reasons. A singleton CompositeSerializer did not make a lot of sense for
me. However, every other serializer in hector is a singleton and IIRC,
there were some concerns about having CompositeSerializer alone as a
"non-singleton". See the attached code for an sample.

On Mon, Feb 4, 2013 at 4:31 PM, Eric Zoerner notifications@github.comwrote:

I'm not referring to the change in AbstractComposite here which is also in
release 1.1-2. I don't believe that that change actually addresses the
original issue (#503 #503)
unless I am missing something.

I am referring to vaidhy's pull request which is a change to
CompositeSerializer that takes a list of serializers at construction time.
I am a bit confused actually, because I don't see how deserialization of a
Composite could possibly work without the serializers being properly set
for each component.

I looked into the NPE I was getting, and this does not occur in master due
to the use of a different version of the google guava library.

Nate's suggestion to use a factory to create a proper CompositeSerializer
is fine, but it doesn't fix the problem with the singleton
CompositeSerializer which is broken for deserialization.

I have just submitted a pull request that contains a modified
CompositeSerializer and also has two unit tests that demonstrate the
problem it fixes. The test that uses the singleton CompositeSerializer
fails, and the test that constructs a CompositeSerializer with the proper
serializers succeeds.

—
Reply to this email directly or view it on GitHubhttps://github.com//pull/503#issuecomment-13071051.