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 #302

Closed
wants to merge 34 commits into from

Conversation

gdiet
Copy link
Contributor

@gdiet gdiet commented Jan 31, 2018

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.

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.

@CLAassistant
Copy link

CLAassistant commented Jan 31, 2018

CLA assistant check
Thank you for your submission, we really appreciate it. Like many open source projects, we ask that you all sign our Contributor License Agreement before we can accept your contribution.
1 out of 2 committers have signed the CLA.

✅ gdiet
❌ Georg Dietrich


Georg Dietrich seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account.
You have signed the CLA already but the status is still pending? Let us recheck it.

@gdiet
Copy link
Contributor Author

gdiet commented Jan 31, 2018

Yes, Travis CI build failed, but :) this is because travis uses scala 2.10.6, and with scala 2.10.7 the tests run green (at least locally). So I propose that the Travis CI is reconfigured to use scala 2.10.7.

@johnynek
Copy link
Collaborator

@gdiet can you bump to modern scalas here:

https://github.com/twitter/chill/blob/develop/.travis.yml

then you should be green?

@johnynek
Copy link
Collaborator

Thanks for this PR, by the way!

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 looks like a great improvement. I appreciate the tests. Thank you.

@@ -121,15 +98,32 @@ class ScalaCollectionsRegistrar extends IKryoRegistrar {
*/
newK
// wrapper array is abstract
.registerClasses(Seq(
Copy link
Collaborator

Choose a reason for hiding this comment

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

can you add all new items at the end of the list? Otherwise, I think we could change the tokens and some people have (ill-advisedly) persisted kryo data into long term storage.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes... I will follow your next proposal and put all new items into a separate registrar.


class RegistrationIdsSpec extends WordSpec with Matchers {
"""
|Projects using chill to long term persist serialized data (for example in event
Copy link
Collaborator

Choose a reason for hiding this comment

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

so this test protects the future, but not people in the past right? They all get broken right?

Could we work around that? Maybe add a new registrar that people can opt in to adding?

Copy link
Contributor Author

@gdiet gdiet Feb 1, 2018

Choose a reason for hiding this comment

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

Right, this would break compatibility for projects that have already used chill for long term storage. Defining a separate registrar is OK for me (although the order of entries in the registrar will be less orderly that way).

I'm thinking along these lines currently:

  • Rename AllScalaRegistrar -> AllScalaRegistrar_0_9_2 for those who need the old registrations.
  • Create a new AllScalaRegistrar that first registers everything from AllScalaRegistrar_0_9_2, then registers all the new items from this PR.

Does this sound OK?

@codecov-io
Copy link

codecov-io commented Feb 1, 2018

Codecov Report

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

Impacted file tree graph

@@             Coverage Diff             @@
##           develop     #302      +/-   ##
===========================================
+ 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...7dde9fa. Read the comment docs.

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.

4 participants