Backward compatible variant for migration from Scala 2.12 to 2.13#467
Backward compatible variant for migration from Scala 2.12 to 2.13#467vigoo wants to merge 4 commits into
Conversation
|
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #467 +/- ##
========================================
Coverage 91.69% 91.69%
========================================
Files 42 42
Lines 1396 1396
Branches 35 35
========================================
Hits 1280 1280
Misses 116 116 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
Thanks for the PR. I'm willing to merge this, but I'd like to have some tests. For instance, one idea would be we could check in a file that has been serialized with one of all these items in 0.9.2 with 2.12 or something, and then assert that we deserialize it. I get very nervous about code that people depend on that has no tests in the repo. |
|
I fully agree and will add some tests! |
regadas
left a comment
regadas
left a comment
There was a problem hiding this comment.
Thanks for this! Would you mind rebasing as well and formatting with scalafmt? +scalafmtAll
| val nameId = classToNameId.get(placeholderType, -1) | ||
| if (nameId != -1) { | ||
| output.writeVarInt(nameId, true) | ||
| return |
There was a problem hiding this comment.
🎨 could we rewrite this without using return?
|
|
||
| override def write(kryo: Kryo, output: Output, `object`: T): Unit = { | ||
| var length: Option[Int] = None | ||
| for (field <- getFields) { |
| val start = kryo.readClassAndObject(input) | ||
| val step = kryo.readClassAndObject(input) | ||
|
|
||
| for (field <- getFields) { |
There was a problem hiding this comment.
🎨 use while? yields better perf.
|
Update: I found some issues, will push fix and tests when have time. |
|
take a look at #514 -- that would break, significantly, binary compatibility, and I assume the serialized data compatibility. you might want to weight in. |
5 participants
The default implementation in Chill does not maintain binary compatibility between Scala version 2.12 and 2.13. I guess this is an intentional decision, as even a separate test data set is provided for the two versions.
Earlier this year we migrated a live service to Scala 2.13 that was using Chill with Akka Persistence and clustering, so had to keep binary compatibility to do a live upgrade. This PR contains an alternative
ScalaKryoInstantiatorfor Scala 2.13 that preserves compatibility with 2.12 in both directions. The compatibility does not mean binary equivalence in all cases but roundtrip serialization between the two Scala versions is possible.This was heavily tested manually and runs since months in production but there are no tests included yet. If this is something you would like to have in the core library as it may help others migrations, I'm happy to add them.