[FOLLOWUP][PR514] Update to use the newer Kryo 5.5.0 #747

roczei · 2023-04-24T11:57:16Z

Hi Chill team,

This is a follow up on this PR: #514

Main changes:

Rebased the PR514 to the latest develop state
Fixed all unit test issues
Set Kryo 5.5.0 version which includes the fix for "Serialization of a generic array of enum" (this fixed the TestVarArgs case class issue in KryoSpec.scala)
Switched to versionless kryo5 package (this fixed all mimaReport issues). If you think this is not the right solution and we have to use the versioned imports, I can drop the last three commits
Added back the chill-storm module

I have already tested with sbt test and all tests passed.

Registration is required by default in Kryo5: https://github.com/EsotericSoftware/kryo/wiki/Migration-to-v5#configuration-changes

This reverts commit 5c8310d.

CLAassistant · 2023-04-24T11:57:29Z

All committers have signed the CLA.

roczei · 2023-04-24T12:04:02Z

FYI: @nicknezis, @johnynek, @regadas

codecov-commenter · 2023-04-24T16:25:15Z

Codecov Report

Patch coverage: 100.00% and project coverage change: +0.01 🎉

Comparison is base (f69dfa0) 91.85% compared to head (8c1bf86) 91.87%.

❗ Current head 8c1bf86 differs from pull request most recent head e657107. Consider uploading reports for the commit e657107 to get more accurate results

Additional details and impacted files

@@             Coverage Diff             @@
##           develop     #747      +/-   ##
===========================================
+ Coverage    91.85%   91.87%   +0.01%     
===========================================
  Files           46       46              
  Lines         1425     1427       +2     
  Branches        28       35       +7     
===========================================
+ Hits          1309     1311       +2     
  Misses         116      116

Impacted Files	Coverage Δ
...la/com/twitter/chill/akka/ActorRefSerializer.scala	`100.00% <ø> (ø)`
...m/twitter/chill/algebird/AlgebirdSerializers.scala	`100.00% <ø> (ø)`
...cala/com/twitter/chill/BijectionEnrichedKryo.scala	`53.33% <ø> (ø)`
.../scala/com/twitter/chill/InjectiveSerializer.scala	`100.00% <ø> (ø)`
...2-/com/twitter/chill/ClassManifestSerializer.scala	`0.00% <ø> (ø)`
...in/scala-2.12-/com/twitter/chill/Traversable.scala	`100.00% <ø> (ø)`
...12-/com/twitter/chill/WrappedArraySerializer.scala	`100.00% <ø> (ø)`
...scala/com/twitter/chill/BigDecimalSerializer.scala	`100.00% <ø> (ø)`
...ain/scala/com/twitter/chill/BitSetSerializer.scala	`100.00% <ø> (ø)`
...n/scala/com/twitter/chill/ClassTagSerializer.scala	`0.00% <ø> (ø)`
... and 17 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

roczei · 2023-05-10T18:16:49Z

I have sent this mail to the chill-user list regarding this pull request:

https://groups.google.com/g/chill-user/c/t5y8xBEjTXY

Hi Chill maintainers / users,

Is this twitter/chill project still active? The latest chill version depends on 4.0.2. Are there any chance for a new chill release which can include the following pull request?

[FOLLOWUP][PR514] Update to use the newer Kryo 5.5.0: #747

We would like to migrate Apache Spark and Apache Livy to Kryo5 and we are blocked due to this missing pull request.

Thanks in advance for any responses!

regadas · 2023-05-10T19:33:20Z

Hey! Things have been a bit busy on my side, but I'll see if I can pick up things here in the coming days.

Thanks for the poke and PR. 🙏

roczei · 2023-05-10T19:41:08Z

Thanks @regadas!

alejandrod-f · 2023-07-07T15:20:12Z

Hi, I hope we can get this soon! It will be a good step forward to unify everything to Kryo 5.

eejbyfeldt · 2023-07-10T08:53:16Z

chill-scrooge/src/main/scala/com/twitter/chill/scrooge/ScroogeThriftStructSerializer.scala

  /* nb: thriftStructClass doesn't actually have type Class[T] it has type Class[_ <: T]
   * this lie is courtesy of the Kryo API
   * */


Looks like this comment should make be dropped now that the kryo api has changed?

Hi @eejbyfeldt,

Thanks for the review! I do not know the answer but if you think we may drop it, I am happy to implement it.

@eejbyfeldt,

Where do you see this that kryo api has changed and we may drop this comment?

It seems to be that kryo went from Class[T] api to a Class[_ <: T] in kryo 5. So the comment basically consistent of two statements

nb: thriftStructClass doesn't actually have type Class[T] it has type Class[_ <: T]

and

this lie is courtesy of the Kryo API

My understanding was that the second statement is not longer true as there is no longer any lying going on (thrift and kryo 5 has matching apis). While the first statement is still true it does not really add any useful information anymore.

While the first statement is still true it does not really add any useful information anymore.

Ok, I have just deleted it.

roczei · 2023-07-17T07:55:17Z

Hey! Things have been a bit busy on my side, but I'll see if I can pick up things here in the coming days.

Hi @regadas,

Sorry to trouble you, but did you a chance to take a look at this pull request? I wonder if you could tell me when can we expect a new chill release which include it? I really appreciate your reply in advance.

mcmoe · 2023-10-03T12:27:56Z

Hello, is there still an intention to merge this PR?

LuciferYang · 2023-12-19T07:43:17Z

build.sbt

@@ -53,7 +53,7 @@ val sharedSettings = Seq(
    "org.scalacheck" %% "scalacheck" % "1.15.2" % "test",
    "org.scalatest" %% "scalatest" % "3.2.15" % "test",
    "org.scalatestplus" %% "scalatestplus-scalacheck" % "3.1.0.0-RC2" % "test",
-    "com.esotericsoftware" % "kryo-shaded" % kryoVersion
+    "com.esotericsoftware" % "kryo" % kryoVersion


According to the description in the Kryo repository, it seems that com.esotericsoftware.kryo:kryo5:5.5.0 should be used?

https://github.com/EsotericSoftware/kryo

Hi @LuciferYang,

I agree with you! Yes, it would be better to use com.esotericsoftware.kryo:kryo5:5.5.0. I have switched to versionless kryo5 package to fix all mimaReport issues. We can switch back to this artifact any time before it would be merged. (I have mentioned this in the PR's description as well). Related commit: 0a6dda1

Currently the main issue is that we cannot go further with this PR because seems like there is no active maintainer of this twitter/chill repository. :-( I am just a simple contributor who opened this PR.

LuciferYang · 2023-12-19T07:45:31Z

Hello ~ Is there any progress on this PR?

nicknezis and others added 19 commits April 11, 2023 19:59

Updated to use the newer Kryo5 shaded jar

4b4f81a

Initial change to use kryo5

826d9c8

More kryo5 updates

c3ac65f

Removed chill-storm

5c8310d

Two more classes that were missed

164d515

A few more fixes

e03e924

Scala Fmt

97bfa32

Reverting this change

803cb11

Test update

fa127fb

Test result updated

dc83e24

More test result updates

76e6d29

Register all classes and fix the unit test issues

5f9b8a1

Registration is required by default in Kryo5: https://github.com/EsotericSoftware/kryo/wiki/Migration-to-v5#configuration-changes

All unit tests passed after rebase

530eb0f

Scala version specific fixes

cde63e4

Scala style fixes

385df3d

Use the latest kryo release: 5.5.0

11d55d2

Switch to versionless kryo artifact

0a6dda1

Revert "Removed chill-storm"

8129bb6

This reverts commit 5c8310d.

Switch to versionless kryo package in BlizzardKryoFactory

e657107

eejbyfeldt reviewed Jul 10, 2023

View reviewed changes

Deleted the old comment in ScroogeThriftStructSerializer.scala

ecb1a92

nicknezis mentioned this pull request Jul 17, 2023

[FLINK-3154][runtime] Upgrade from Kryo v2 + Chill 0.7.6 to Kryo v5 w… apache/flink#22660

Open

LuciferYang reviewed Dec 19, 2023

View reviewed changes

pan3793 mentioned this pull request Aug 8, 2024

[SPARK-49147][CORE] Mark KryoRegistrator with DeveloperApi interface apache/spark#47657

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[FOLLOWUP][PR514] Update to use the newer Kryo 5.5.0 #747

[FOLLOWUP][PR514] Update to use the newer Kryo 5.5.0 #747

roczei commented Apr 24, 2023 •

edited

Loading

CLAassistant commented Apr 24, 2023 •

edited

Loading

roczei commented Apr 24, 2023 •

edited

Loading

codecov-commenter commented Apr 24, 2023

roczei commented May 10, 2023 •

edited

Loading

regadas commented May 10, 2023

roczei commented May 10, 2023

alejandrod-f commented Jul 7, 2023

eejbyfeldt Jul 10, 2023

roczei Jul 11, 2023

roczei Jul 13, 2023

eejbyfeldt Jul 13, 2023 •

edited

Loading

roczei Jul 14, 2023

roczei commented Jul 17, 2023

mcmoe commented Oct 3, 2023

LuciferYang Dec 19, 2023

roczei Dec 19, 2023 •

edited

Loading

LuciferYang commented Dec 19, 2023

[FOLLOWUP][PR514] Update to use the newer Kryo 5.5.0 #747

Are you sure you want to change the base?

[FOLLOWUP][PR514] Update to use the newer Kryo 5.5.0 #747

Conversation

roczei commented Apr 24, 2023 • edited Loading

CLAassistant commented Apr 24, 2023 • edited Loading

roczei commented Apr 24, 2023 • edited Loading

codecov-commenter commented Apr 24, 2023

Codecov Report

roczei commented May 10, 2023 • edited Loading

regadas commented May 10, 2023

roczei commented May 10, 2023

alejandrod-f commented Jul 7, 2023

eejbyfeldt Jul 10, 2023

Choose a reason for hiding this comment

roczei Jul 11, 2023

Choose a reason for hiding this comment

roczei Jul 13, 2023

Choose a reason for hiding this comment

eejbyfeldt Jul 13, 2023 • edited Loading

Choose a reason for hiding this comment

roczei Jul 14, 2023

Choose a reason for hiding this comment

roczei commented Jul 17, 2023

mcmoe commented Oct 3, 2023

LuciferYang Dec 19, 2023

Choose a reason for hiding this comment

roczei Dec 19, 2023 • edited Loading

Choose a reason for hiding this comment

LuciferYang commented Dec 19, 2023

roczei commented Apr 24, 2023 •

edited

Loading

CLAassistant commented Apr 24, 2023 •

edited

Loading

roczei commented Apr 24, 2023 •

edited

Loading

roczei commented May 10, 2023 •

edited

Loading

eejbyfeldt Jul 13, 2023 •

edited

Loading

roczei Dec 19, 2023 •

edited

Loading