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

Pre-registering certain standard classes to better support long term persistence scenarios #303

Merged
merged 32 commits into from
Feb 21, 2018

Conversation

gdiet
Copy link
Contributor

@gdiet gdiet commented Feb 2, 2018

This PR replaces #302, there have been problems caused by commits containing an incorrect committer's email address.

This PR is about registering standard scala classes that are likely to be used when long term persisting data, for example in event stores:

When using kryo/chill for long term storage, it is advisable to run Kryo with setRegistrationRequired(true). However, a number of standard scala classes that are useful in such scenarios are not pre-registered in chill, most of them located in scala.collections.immutable. This has puzzled others as well, see for example the chill issue #227 which is fixed by this PR.

Some of the classes I have added to the standard registration in this pull request:

scala.None
scala.collection.immutable.::
scala.collection.immutable.Nil
scala.collection.immutable.ListSet.EmptyListSet
scala.collection.immutable.Range
scala.collection.mutable.WrappedArray.ofRef
int[]

To better support the long term persistence scenario, I have added the RegistrationIdsSpec, which will fail if the pre-registered IDs change, because changing them should be avoided if possible to ensure backwards compatibility. Or, if changing pre-registered IDs is really inevitable, a compatibility note should be added to the release notes.

For projects that already depend on the class ID registration present in chill 0.9.2, there is a compatibility registrar AllScalaRegistrar_0_9_2 defined.

If this PR is accepted, I plan to complement it with two more PRs: One PR for the chill documentation with notes on how kryo/chill can be used for long term persisting events (lessons learned). The other PR would be another spec ensuring that the binary representation of serialized classes is not changed unintendedly.

@codecov-io
Copy link

Codecov Report

Merging #303 into develop will increase coverage by 4.27%.
The diff coverage is 98.24%.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop     #303      +/-   ##
===========================================
+ Coverage    47.86%   52.13%   +4.27%     
===========================================
  Files           41       42       +1     
  Lines         1262     1314      +52     
  Branches        38       38              
===========================================
+ Hits           604      685      +81     
+ Misses         658      629      -29
Impacted Files Coverage Δ
...la/com/twitter/chill/ClassManifestSerializer.scala 0% <0%> (ø) ⬆️
...ain/scala/com/twitter/chill/StreamSerializer.scala 100% <100%> (ø)
...cala/com/twitter/chill/ScalaKryoInstantiator.scala 99.2% <100%> (+0.52%) ⬆️
...ain/scala/com/twitter/chill/TupleSerializers.scala 19.04% <0%> (+4.6%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0f35017...1960f57. Read the comment docs.

@gdiet
Copy link
Contributor Author

gdiet commented Feb 8, 2018

@johnynek thank you for your comments to #302 - this PR replaces that one, and I have updated things to address the concerns you mentioned.

Copy link
Collaborator

@johnynek johnynek left a comment

Choose a reason for hiding this comment

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

This is great, thanks!

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

Successfully merging this pull request may close these issues.

3 participants