Cross-build (most projects) for Scala 2.12.0 by JoshRosen · Pull Request #253 · twitter/chill

JoshRosen · 2016-04-08T22:36:24Z

This patch allows several of the chill modules to be built for Scala 2.12.0.

Several subprojects, such as chill-akka and chill-bijection, cannot currently be published for 2.12 because their dependencies have not been published yet. To work around this, I used the sbt-doge plugin, which overrides ++ and + to respect subprojects' crossScalaVersions settings, allowing you to run

> ; + clean; + test; + publishLocal

to test and publish for all Scala versions.

Refs #252.

References: http://movingfulcrum.tumblr.com/post/80826553604/asm-framework-50-the-missing-migration-guide apache/spark#9512

JoshRosen · 2016-04-08T23:07:17Z

java.lang.NoClassDefFoundError: java/lang/invoke/SerializedLambda
    at com.esotericsoftware.kryo.serializers.ClosureSerializer.<clinit>(ClosureSerializer.java:42)
    at com.twitter.chill.AllScalaRegistrar.apply(ScalaKryoInstantiator.scala:207)
    at com.twitter.chill.ScalaKryoInstantiator.newKryo(ScalaKryoInstantiator.scala:97)
    at com.twitter.chill.avro.AvroSerializerSpec$$anonfun$5.apply(AvroSerializerSpec.scala:34)
    at com.twitter.chill.avro.AvroSerializerSpec$$anonfun$5.apply(AvroSerializerSpec.scala:34)
    at com.twitter.chill.package$$anon$1.newKryo(package.scala:30)
    at com.twitter.chill.KryoPool$2.newInstance(KryoPool.java:59)
    at com.twitter.chill.KryoPool$2.newInstance(KryoPool.java:57)
    at com.twitter.chill.ResourcePool.borrow(ResourcePool.java:37)

It looks like I need to add logic to not register ClosureSerializer unless we're running on a Java 8 JRE.

… do this).

JoshRosen · 2016-04-10T01:41:53Z

    k.register(boxedUnit.getClass, new SingletonSerializer(boxedUnit))
    PackageRegistrar.all()(k)
+
+    // Enable Java 8 lambda serialization only if we are running on a Java 8 JRE:


Quick question: what's the best place to register this? And what happens if it already happens to have been registered by some other means?

I think we should make a Java8ClosureRegistrar that is an IKryoRegistrar: https://github.com/twitter/chill/blob/develop/chill-java/src/main/java/com/twitter/chill/IKryoRegistrar.java and it should be a no-op when we are not on java8 (as you have below).

See for instance the PackageRegistrar.all() above.

Good idea; will do.

Whoops, forgot about this comment until now. I've created a Java8ClosureRegistrar to abstract away this code.

johnynek · 2016-04-12T19:31:22Z

looks great. I don't want to merge pointing to a SNAPSHOT so whenever we can fix that, we should merge.

See https://issues.scala-lang.org/browse/SI-6710 for context. This is necessary for Scala 2.12.0-M5.

lrytz · 2016-04-20T13:54:31Z


    // use the singleton serializer for boxed Unit
-    val boxedUnit = scala.Unit.box(())
+    val boxedUnit = scala.runtime.BoxedUnit.UNIT


this won't be necessary in M5, fixed in scala/scala#5100

johnynek · 2016-10-11T00:40:38Z

do you have any interest in this still?

The PR was red, and I didn't look into it much till now. It looks like it may have been a transient error with Travis. I restarted it.

johnynek · 2016-10-11T20:34:03Z

if you can merge with develop and bump to a non-snapshot version of kryo 3.x.y I'll merge this.

jodersky · 2016-11-03T00:24:05Z

I think kryo 3.1 is required as it solves an issue with registering closure serializers (EsotericSoftware/kryo#299). Unfortunately 3.1 was never released, and the project went straight ahead to 4.0.0.
#258 would fix the issue and unblock this PR

magro · 2016-11-03T09:52:55Z

@johnynek Out of interest, is there anything holding you back to switch to kryo 4.0.0?

johnynek · 2016-11-04T02:54:13Z

So, sadly, many people against our recommendation have used kryo
serialization for long term storage. This creates a large anxiety for such
users to upgrade versions due to fear they are going to subtly break their
data pipelines.

I'm happy to help merge and release chill for later Kryos (basically we
increment a version each time we do that). It's a pain to have several
concurrent versions running though.

So, I'm happy to make a new version of chill with the latest kryo.
On Wed, Nov 2, 2016 at 23:52 Martin Grotzke notifications@github.com
wrote:

@johnynek https://github.com/johnynek Out of interest, is there
anything holding you back to switch to kryo 4.0.0?

—
You are receiving this because you were mentioned.

Reply to this email directly, view it on GitHub
#253 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/AAEJdv3ZM6qNb3ATa_PKeSskW0sFgUXEks5q6a74gaJpZM4IDcTm
.

johnynek · 2016-11-04T02:56:27Z

Do you want to bump this up to 2.12.0?

magro · 2016-11-04T12:35:06Z

@johnynek Ok, I fully understand this. Thanks for taking the route to kryo 4!

JoshRosen · 2016-11-04T21:16:58Z

I just updated this to build against 2.12.0 final. Once #258 is merged I can update this to use Kryo 4. @johnynek, I'm going to click the "Allow edits from maintainers" button in case you want to update and merge this after #258 goes in. Otherwise, I can do it myself.

johnynek · 2016-11-06T23:28:06Z

okay, merged #258. if you merge develop, I'll merge this.

JoshRosen · 2016-11-08T02:07:02Z

This is now passing tests and I think should be ready for a final review. Let me know if you'd like me to make any changes.

sritchie-stripe · 2016-11-10T15:44:20Z

LGTM, but let's wait on @johnynek for the final shipit.

Once scala 2.12 ships do we have any plans to remove support for 2.10, or are we committed to three minor versions?

johnynek · 2016-11-10T17:19:43Z

Okay. This looks good. Let's merge this and this can be the 0.9.x branch for Kryo 4.x

JoshRosen added 5 commits March 28, 2016 17:12

WIP

b3d4d50

Use Kryo 3.1.0-SNAPSHOT; register ClosureSerializer

99e51a0

Update ClosureCleaner to use ASM 5 APIs:

c98f80e

References: http://movingfulcrum.tumblr.com/post/80826553604/asm-framework-50-the-missing-migration-guide apache/spark#9512

Use sbt-doge to exclude certain subprojects from the 2.12 build.

97573d3

Remove testOptions change.

26e8bd0

JoshRosen added 3 commits April 8, 2016 16:15

Only register ClosureSerializer when running on Java 8 JDK.

c11010f

Update to use sbt-doge's +++ "strict aggregation" operator.

a17fd8e

Workaround to fix PriorityQueueTest (there's probably a better way to…

3376e7b

… do this).

JoshRosen reviewed Apr 10, 2016
View reviewed changes

Replace Unit.box() call by scala.runtime.BoxedUnit.UNIT

3636f1c

See https://issues.scala-lang.org/browse/SI-6710 for context. This is necessary for Scala 2.12.0-M5.

lrytz reviewed Apr 20, 2016
View reviewed changes

jodersky mentioned this pull request Sep 9, 2016

Upgrade to Kryo 4.0.0 #258

Merged

jodersky mentioned this pull request Nov 4, 2016

Add Scala 2.12 support #266

Closed

Update to 2.12.0 final.

25e982b

JoshRosen changed the title ~~[WIP] Cross-build (most projects) for Scala 2.12.0-M4~~ [WIP] Cross-build (most projects) for Scala 2.12.0 Nov 4, 2016

JoshRosen and others added 2 commits November 6, 2016 17:13

Merge remote-tracking branch 'origin/develop' into scala-2.12

c99ec53

Undo BoxedUnit workaround since it's no longer necessary.

753f03c

JoshRosen changed the title ~~[WIP] Cross-build (most projects) for Scala 2.12.0~~ Cross-build (most projects) for Scala 2.12.0 Nov 7, 2016

Create Java8ClosureRegistrar

074dc8d

JoshRosen force-pushed the scala-2.12 branch from 43d2729 to 074dc8d Compare November 7, 2016 01:36

Fix Java8ClosureRegistrar; standardize formatting.

b2ec04e

johnynek merged commit da51348 into twitter:develop Nov 10, 2016

JoshRosen deleted the scala-2.12 branch November 10, 2016 21:50

piyushnarang mentioned this pull request Nov 28, 2016

Add support for Scala 2.12.0 twitter/bijection#258

Merged

Conversation

JoshRosen commented Apr 8, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

JoshRosen commented Apr 8, 2016

Uh oh!

JoshRosen Apr 10, 2016

Choose a reason for hiding this comment

Uh oh!

johnynek Apr 12, 2016

Choose a reason for hiding this comment

Uh oh!

JoshRosen Apr 12, 2016

Choose a reason for hiding this comment

Uh oh!

JoshRosen Nov 7, 2016

Choose a reason for hiding this comment

Uh oh!

johnynek commented Apr 12, 2016

Uh oh!

lrytz Apr 20, 2016

Choose a reason for hiding this comment

Uh oh!

johnynek commented Oct 11, 2016

Uh oh!

johnynek commented Oct 11, 2016

Uh oh!

jodersky commented Nov 3, 2016

Uh oh!

magro commented Nov 3, 2016

Uh oh!

johnynek commented Nov 4, 2016

Uh oh!

johnynek commented Nov 4, 2016

Uh oh!

magro commented Nov 4, 2016

Uh oh!

JoshRosen commented Nov 4, 2016

Uh oh!

johnynek commented Nov 6, 2016

Uh oh!

JoshRosen commented Nov 8, 2016

Uh oh!

sritchie-stripe commented Nov 10, 2016

Uh oh!

johnynek commented Nov 10, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

JoshRosen commented Apr 8, 2016 •

edited

Loading