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

Missing nonempty list in collection registration #227

Open
rcoh opened this issue Apr 14, 2015 · 7 comments
Open

Missing nonempty list in collection registration #227

rcoh opened this issue Apr 14, 2015 · 7 comments

Comments

@rcoh
Copy link

rcoh commented Apr 14, 2015

The following will fail if registration is required:

serializer.serialize(List(1,2,3))

This is because the collections list is missing :: and only has the empty list:

 // List is a sealed class, so there are only two subclasses:
      .forTraversableSubclass(List.empty[Any])

The fix is just to add list, but it needs to be added to the end or all of the registration numbers will change and it will break clients.

@rcoh rcoh changed the title Missing scala.collection.:: in registration Missing nonempty list in collection registration Apr 14, 2015
@ianoc
Copy link
Collaborator

ianoc commented Apr 14, 2015

We don't expect Chill serialization to be stable across jvm launches/recompiles so it should be fine to add it in to the natural place in the constructor. Would happily take a PR

@peter-empen
Copy link

Could you please elaborate on the above statement "We don't expect Chill serialization to be stable across jvm launches/recompiles"?
We do expect registration numbers to be stable. How do you expect users to upgrade Chill otherwise?
Btw., is this issue still valid?

@ianoc
Copy link
Collaborator

ianoc commented Dec 19, 2017

Chill was never designed to be used as a long term serialization format, since the underlying kryo itself wasn't really designed for this. Kryo has become a bit more stable over time in this regard but i'd still not recommend ever using kryo/chill as a long term format.

The common place this is used as a magic/auto-serializer between hosts in a distributed system. Its used in Scalding/Spark/Summingbird for this, Storm uses kryo too iirc.

@peter-empen
Copy link

Our use case spans Akka Distributed and Event Stores.
We are confident that, once registry numbers are dealt with care, we do achieve kinda long-term format. For this purpose, we certainly work with RegistrationRequired.

@ianoc
Copy link
Collaborator

ianoc commented Dec 19, 2017

Chill has no tests requiring these numbers are constant from build to build. So i think you can propose a different tack and all the tests here. But barring that if the tests don't require it I don't think you should rely on this. Using something like protobuf/thrift/etc.. that all aim for backwards compatibility/LTS.

(Side note where I am now an online event store was done using chill/kryo for serialization persistance, and its been a total pain since we can't easily upgrade kryo in anything touching its classpath.)

@peter-empen
Copy link

We do have these tests already.
Yeh, we did realize upgrading Kryo over major releases won't be possible. Why do you think we will be hit by this restriction seriously? What about using current Kryo with current feature set for some years?

@ianoc
Copy link
Collaborator

ianoc commented Dec 19, 2017

Well we have as our repos got bigger, more use cases. Your code paths can never touch another pre-compiled library using a newer kryo that isn't binary compatible unless your going down some shading or other routes. Anyway, its all a tradeoff/choice, just not one i'd recommend/make. It can be very hard to undo what you have serialized even to in memory stores. Kryo I don't think can/will necessarily validate what its deserialized isn't garbage too.

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

No branches or pull requests

3 participants