Skip to content

Ensure the binary representation of serialized classes is not changed unintendedly #310

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.

Sign up for GitHub

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

Jump to bottom
Merged
johnynek merged 52 commits into from
Jun 18, 2018
Merged

Ensure the binary representation of serialized classes is not changed unintendedly #310

johnynek merged 52 commits into from
Jun 18, 2018

Conversation

@gdiet
Copy link
Contributor

@gdiet gdiet commented May 14, 2018

This PR is continuing the idea from PR #303 as follows:

When using kryo/chill for long term storage, it is necessary that classes are serialized in a compatible way when updating to a newer Kryo or Scala version. To ensure this, I have added a spec ensuring that the binary representation of serialized classes is not changed unintendedly due to changes in Kryo or Scala. If changes to the binary representation can't be avoided, at least a compatibility note should be added to the release notes so projects using the library know beforehand whether it's safe to upgrade or not.

The spec I have added is still incomplete and fails for Scala 2.10. This PR is to discuss the spec and its findings:

  1. Does this spec in general make sense to you? Otherwise, I would not continue to work on it, would be wasted time.

  2. For Scala 2.10, this spec fails, because (see lines 47ff) the binary representation of Wrappers$IteratorWrapper has changed between Scala 2.10.7 and Scala 2.11.12. The binary representation in line 51 was created with Scala 2.12.4 and can't be decoded with Scala 2.10.7, a forward compatibility problem. (Note that I have not yet been able to find any backward compatibility problem up to now.) For this specific case, I could write the test in a way that this example is skipped when running on Scala < 2.11. Would that be OK from your point of view?

Please comment ...

gdiet added 30 commits January 9, 2018 12:06
@gdiet
Update to latest scala versions
a821f7f
@gdiet
Fix for deprecation warning
24a3b7c
@gdiet
Fixed typo in spec and removed unused import
0924a71
@gdiet
Added test to ensure long term stability of registration IDs
39afb4a
@gdiet
Renamed spec
2cc50bf
@gdiet
Require registration for empty set
234f737
@gdiet
Registration for empty set
de36cbe
@gdiet
Require registration for hash sets
af986af
@gdiet
Registration for hash sets
8266b03
@gdiet
Require registration for list set
65e92ce
@gdiet
Registration for list set
d7c4d0e
@gdiet
Require registration for list, queue, range
0e163b7
@gdiet
Registration for list, queue, range
6d27088
@gdiet
Require registration for hash maps, list maps, None, and arrays
71ab736
@gdiet
Registration for hash maps, list maps, None, and arrays
38dbd75
@gdiet
Require registration for wrapped arrays
c55a179
@gdiet
Registration for wrapped arrays
45eb733
@gdiet
Require registration for wrapped strings
8c48dd7
@gdiet
Registration for wrapped strings
8e8cde6
@gdiet
Registration for streams
35cc7cd
@gdiet
Registration for TreeSet and TreeMap
01e75fb
@gdiet
Added missing registrations to test
98d6da9
@gdiet
Registration for the empty map
0f9a268
@gdiet
Added missing registrations to test
4533516
@gdiet
found a problem when serializing streams
1bb412d
@gdiet
fixed stream serializer
d2a6941
@gdiet
finished test examples
eb229fa
@gdiet
bumping up scala versions for travis ci
a327ed2
@gdiet
revert ScalaKryoInstantiator to redesign changes in backward compatible
0b90a4e
@gdiet
introduced 0.9.2 compatibility registrar
6d25303
gdiet added 11 commits February 2, 2018 13:50
@gdiet
re-added registrations, this time with the backward compatibility layer
810bdeb
@gdiet
improved scaladoc for changed and new stuff
1960f57
@gdiet
Merge remote-tracking branch 'upstream/develop' into PR
a030f9a
@gdiet
first idea for SerializedExamplesOfStandardDataSpec
862bf33 
(cherry picked from commit 19cfef1)
@gdiet
getting first examples (boxed primitives) to work in the new spec
b28c281 
(cherry picked from commit a389b8f)
@gdiet
started making SerializedExamplesOfStandardDataSpec a working spec
720f02e 
(cherry picked from commit 1c0f50c)
@gdiet
working around the 'can't use equals' problem for the spec
962164c 
(cherry picked from commit 6bb3fce)
@gdiet
added more examples to SerializedExamplesOfStandardDataSpec
08d2eb3 
(cherry picked from commit f4dc040)
@gdiet
compile compatibility to Scala 2.11 and 2.10
1087c72
@gdiet
added explicit example for serialization incompatibility between scal…
edd37cb 
…a versions
@gdiet
final touches to have something that can be discussed as pull request
9c4f375
@johnynek
Copy link
Collaborator

johnynek commented May 14, 2018

Thanks for the PR. This looks like a fine thing to add to me.

That said, using kryo for long term storage but depending on a remote library to set the order of the tokens seems really dangerous.

I wouldn't use kryo for long term storage full stop....

But I am happy to take a PR so we don't needlessly break people.

gdiet added 11 commits June 18, 2018 11:10
@gdiet
Merge branch 'develop' of https://github.com/twitter/chill into PR
de943cd
@gdiet
Added Scala 2.10.x exception to SerializedExamplesOfStandardDataSpec
af99d89 
(cherry picked from commit 2a40471)
@gdiet
SerializedExamplesOfStandardDataSpec added more examples
7c21af4 
(cherry picked from commit c6e5b05)
@gdiet
SerializedExamplesOfStandardDataSpec examples for tupleN
fbc569a 
(cherry picked from commit 2ac764b)
@gdiet
SerializedExamplesOfStandardDataSpec examples for specialized Tuple1
fc85130 
(cherry picked from commit 2bf66b8)
@gdiet
SerializedExamplesOfStandardDataSpec examples for specialized Tuple2
8d2c718 
(cherry picked from commit 4e5b504)
@gdiet
updated to Scala 2.12.6
1ffeef9 
(cherry picked from commit 22bfcda)
@gdiet
more examples in SerializedExamplesOfStandardDataSpec
eeb005b 
(cherry picked from commit 8531137)
@gdiet
finished implementing examples for SerializedExamplesOfStandardDataSpec
1e1835c 
(cherry picked from commit bcf0176)
@gdiet
one additional case where serialization changed with scala version
13c5931 
(cherry picked from commit 81fdfaf)
@gdiet
added serialization examples for BigDecimal and VolatileByteRef that …
da15efb 
…have been added by PR 312
@johnynek
Copy link
Collaborator

johnynek commented Jun 18, 2018

Thanks! this looks good.

@johnynek johnynek merged commit 84ab358 into twitter:develop Jun 18, 2018
@gdiet
Copy link
Contributor Author

gdiet commented Jun 19, 2018 •
edited
Loading

Here are some findings of this PR - for future reference. Currently, there are already some known issues when using kryo/chill for long term persisting data:

  1. Serializing SimpleDateFormat("") yields about 40.000 bytes of binary representation, and creating another instance and serializing it yields a slightly different binary representation - there might be more issues hidden there...

  2. The serialized form of the following classes differs between Scala 2.12.x and earlier Scala versions:

Compard to Scala 2.10:

scala.collection.convert.Wrappers$IteratorWrapper
Example: JavaConverters.asJavaIteratorConverter(Iterator(2)).asJava

scala.collection.immutable.Range$Inclusive
Example: new Range.Inclusive(3, 6, 1)

scala.collection.immutable.Range
Example: collection.immutable.Range(1, 3)

Compard to Scala 2.11:

scala.collection.immutable.Range$Inclusive
Example: new Range.Inclusive(3, 6, 1)

scala.collection.immutable.Range
Example: collection.immutable.Range(1, 3)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@gdiet @johnynek