chill icon indicating copy to clipboard operation
chill copied to clipboard

Missing nonempty list in collection registration

Open rcoh opened this issue 10 years ago • 7 comments

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 avatar Apr 14 '15 23:04 rcoh

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

ianoc avatar Apr 14 '15 23:04 ianoc

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?

peter-empen avatar Dec 19 '17 08:12 peter-empen

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.

ianoc avatar Dec 19 '17 14:12 ianoc

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.

peter-empen avatar Dec 19 '17 15:12 peter-empen

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.)

ianoc avatar Dec 19 '17 15:12 ianoc

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?

peter-empen avatar Dec 19 '17 15:12 peter-empen

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.

ianoc avatar Dec 19 '17 15:12 ianoc