Support Scala 2.12 #724

ttim · 2017-06-02T08:30:58Z

This PR bumps dependencies versions to most recent ones with Scala 2.12 support.
Algebird fixes based on #715.

This PR also removes support for Scala 2.10, because scalding doesn't support it.

ttim · 2017-06-02T08:32:55Z

This PR will be mergeable as soon as we release scalding 0.17.1 with fix for KryoHadoop twitter/scalding#1685.

ttim · 2017-07-11T00:30:19Z

Now, when scalding release is out we can merge this 2.12 upgrade for SB. What do you think @pankajroark @piyushnarang @johnynek ?

ttim · 2017-07-11T00:38:00Z

Update: false alarm, tests are failing, digging into this now.

…mestampSuccessible

piyushnarang

Looks good to me once you have the build green.

piyushnarang · 2017-07-11T17:25:16Z

build.sbt

@@ -188,7 +185,7 @@ def youngestForwardCompatible(subProj: String) =
 // Uncomment after release.
 //  Some(subProj)
 //    .filterNot(unreleasedModules.contains(_))


should we delete these commented lines?

no, we need to uncomment after each version bump. Deleting seems like a good way to keep making mima mistakes like the scalding one.

johnynek · 2017-07-12T02:07:08Z

looks like a legit failure:

https://travis-ci.org/twitter/summingbird/jobs/252265982#L3657

Caused by: com.esotericsoftware.kryo.KryoException: java.lang.RuntimeException: Could not serialize lambda

Serialization trace:

fn (com.twitter.scalding.typed.FlatMappedFn)

	at com.esotericsoftware.kryo.serializers.ObjectField.read(ObjectField.java:144)

	at com.esotericsoftware.kryo.serializers.FieldSerializer.read(FieldSerializer.java:551)

	at com.esotericsoftware.kryo.Kryo.readClassAndObject(Kryo.java:790)

	at com.twitter.chill.SomeSerializer.read(SomeSerializer.scala:25)

	at com.twitter.chill.SomeSerializer.read(SomeSerializer.scala:19)

	at com.esotericsoftware.kryo.Kryo.readClassAndObject(Kryo.java:790)

	at com.twitter.chill.SerDeState.readClassAndObject(SerDeState.java:61)

	at com.twitter.chill.KryoPool.fromBytes(KryoPool.java:94)

	at com.twitter.chill.Externalizer.fromBytes(Externalizer.scala:145)

	at com.twitter.chill.Externalizer.maybeReadJavaKryo(Externalizer.scala:158)

	at com.twitter.chill.Externalizer.readExternal(Externalizer.scala:148)

	at java.io.ObjectInputStream.readExternalData(ObjectInputStream.java:1840)

This could be that we don't have the right chill version? It could be this exposes a bug in chill.

ttim · 2017-07-12T20:54:19Z

@johnynek yes, it is legit =( Unfortunately this time it comes from scalacheck and it's same issue as we saw in chill, yes - too many lambdas in one class. I tried to workaround this by creating this lambdas in summingbird code but there are couple of things which makes everything more complicated - they declare everything as private[scalacheck] (thanks not in general) and most of the classes are sealed. At some point I decided to stop and now I'm going to make a PR to scalacheck instead. If they do release quickly we can use new version otherwise I will continue to work on workaround.

non · 2017-07-12T21:41:39Z

@ttim I'm happy to help get this fixed, either in this repo or ScalaCheck? What were you planning to do to work around this? Restructure the ScalaCheck code a bit? Create your own custom Gen instances?

ttim · 2017-07-12T22:12:04Z

@non that's awesome! If you can also help with releasing new version it would be twice awesome =)

In short term for SB I thought just to add code like this:

object ArbitraryWorkaround {
 implicit val f1: Arbitrary[Int => Int] = Arbitrary(Gen.const(x => x * 2))
 implicit val f2: Arbitrary[Int => List[(Int, Int)]] = Arbitrary(Gen.const(x => List((x, x * 3))))
 implicit val f3: Arbitrary[Int => Option[Int]] = Arbitrary(Gen.const(x => {
   if (x % 2 == 0) None else Some(x * 4)
 }))
 implicit val f4: Arbitrary[Int => List[Int]] = Arbitrary(Gen.const(x => List(x * 5)))
 implicit val f5: Arbitrary[((Int, (Int, Option[Int]))) => List[(Int, Int)]] = Arbitrary(Gen.const {
   case (x, (y, optZ)) => List((x, y), (x, optZ.getOrElse(42)))
 })
 implicit val f6: Arbitrary[((Int, Int)) => List[(Int, Int)]] = Arbitrary(Gen.const(x => List(x, x)))
}

But if we can do release reasonably soon I will do PR to scalacheck as soon as possible (today/tomorrow). Plan is to add a test on that + divide ArbitraryArities and GenArities into two files. Wdyt?

non · 2017-07-12T22:18:04Z

@ttim My suggestion is that if the above work-around solves the issue for Summingbird we start there.

I'm happy to try to expedite the suggested change to ScalaCheck, but unfortunately the arities files are traits right now, so restructuring them will break binary compatibility. This doesn't mean we can't do it, but might make a quick release more difficult.

non · 2017-07-12T22:20:00Z

(Also, if you'd prefer help I can make a branch off of this one and try the approach out myself later tonight.)

ttim · 2017-07-12T22:30:46Z

@non let's do this then. I'll merge my workaround. Meanwhile: this is a scala issue - https://issues.scala-lang.org/browse/SI-10232.
Here is repro I used:

import org.scalacheck.Arbitrary._
val f = arbitrary[Int => Int]
val outputStream = new java.io.ByteArrayOutputStream()
val objectOutputStream = new java.io.ObjectOutputStream(outputStream)
objectOutputStream.writeObject(f)
objectOutputStream.close
val inputStream = new java.io.ByteArrayInputStream(outputStream.toByteArray)
val objectInputStream = new java.io.ObjectInputStream(inputStream)
objectInputStream.readObject()

Causes:

Caused by: java.lang.BootstrapMethodError: too many bootstrap method arguments
  at java.lang.invoke.CallSite.makeSite(CallSite.java:320)
  at java.lang.invoke.MethodHandleNatives.linkCallSiteImpl(MethodHandleNatives.java:307)
  at java.lang.invoke.MethodHandleNatives.linkCallSite(MethodHandleNatives.java:297)
  at org.scalacheck.GenArities.$deserializeLambda$(GenArities.scala)

ttim · 2017-07-12T22:35:10Z

@non created a bug in scalacheck repo typelevel/scalacheck#342, thank you very much for looking on this!

johnynek · 2017-07-12T23:55:09Z

build.sbt

 val hadoopVersion = "1.2.1"
 val junitVersion = "4.11"
 val log4jVersion = "1.2.16"
 val novocodeJunitVersion = "0.10"
 val scalaCheckVersion = "1.13.4"
 val scalatestVersion = "3.0.1"
-val scaldingVersion = "0.16.1-RC3"
+val scaldingVersion = "0.17.1"


can we roll back to 0.17.0 here or wait for 0.17.2 due to the recently discovered semver issue with 0.17.1 cc @piyushnarang

0.17.0 breaks SB tests so we can't roll back. I'm going to publish 0.17.2 today.

johnynek

comment on scalding versioning, but otherwise looks fine.

non · 2017-07-13T03:45:43Z

One other thing to note: it seems like the bug suggests that using -Ydelamdafy:inline is a work-around, so it's possible we could include that in the scalac arguments used for testing to avoid the Scala bug.

(Thanks for giving me a repro -- I'll start seeing what I have to do to fix this issue in ScalaCheck.)

ttim · 2017-07-13T04:14:54Z

@non I'm afraid -Ydelambdafy:inline should be used at the scalacheck compile time, so it's not possible to use as a workaround =(

travisbrown-stripe · 2017-07-13T17:48:22Z

I've got a fix on the ScalaCheck side that should let us publish an 0.13.6 release there that will resolve the issue without the workaround here.

ttim · 2017-07-21T20:03:33Z

Scalacheck release isn't out therefore I think to merge this PR now, and I created issue to remove it later as soon as scalacheck will be released - #741.
Wdyt @johnynek @pankajroark @piyushnarang ?

travisbrown-stripe · 2017-07-21T20:10:58Z

I think merging this now and removing the workaround later sounds reasonable, but I think @non is planning to publish the ScalaCheck release either this weekend or next week.

codecov-io · 2017-07-21T20:11:58Z

Codecov Report

Merging #724 into develop will increase coverage by 0.04%.
The diff coverage is 100%.

@@             Coverage Diff             @@
##           develop     #724      +/-   ##
===========================================
+ Coverage    72.21%   72.26%   +0.04%     
===========================================
  Files          153      154       +1     
  Lines         3736     3742       +6     
  Branches       204      209       +5     
===========================================
+ Hits          2698     2704       +6     
  Misses        1038     1038

Impacted Files	Coverage Δ
.../twitter/summingbird/scalding/VersionedState.scala	`0% <ø> (ø)`	⬆️
...cala/com/twitter/summingbird/batch/Timestamp.scala	`65.71% <100%> (ø)`
...cala/com/twitter/summingbird/example/Storage.scala	`77.77% <100%> (+1.3%)`	⬆️
.../com/twitter/summingbird/ArbitraryWorkaround.scala	`100% <100%> (ø)`
...ain/scala/com/twitter/summingbird/TestGraphs.scala	`86.62% <0%> (-1.92%)`	⬇️
...ala/com/twitter/summingbird/scalding/Service.scala	`76.92% <0%> (-1.54%)`	⬇️

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 fecdfa3...b860fb9. Read the comment docs.

ttim · 2017-07-21T20:17:43Z

@travisbrown-stripe I prefer to keep it merged and do not put any pressure on scalacheck =) I'll create removal PR immediately after merge thou.

Timur Abishev added 5 commits June 2, 2017 01:07

Remove scala 2.10 and add scala 2.12

6ed856f

Bump versions of deps

e91ffe6

Fix algebird related issues based on twitter#715

473517d

Fix finagle related compilation error

67d32fb

Fix ClientStoreLaws

99fbe79

ttim mentioned this pull request Jun 2, 2017

Support Scala 2.12 #721

Closed

Timur Abishev added 4 commits June 25, 2017 22:40

Merge remote-tracking branch 'upstream/develop' into scala_2.12

0ab0998

Merge remote-tracking branch 'upstream/develop' into scala_2.12

856c9e1

Merge remote-tracking branch 'upstream/develop' into scala_2.12

68dcc4e

Bump scalding version

a6bd6e3

ttim requested review from johnynek, pankajroark and piyushnarang July 11, 2017 00:28

Timur Abishev added 3 commits July 10, 2017 18:03

Rename TimeStamp.scala to Timestamp.scala and fix serialization of ti…

8a693b7

…mestampSuccessible

Remove usage of JavaConversions

93d48ef

Trying to fix travis scalacheck fail

4977e9b

piyushnarang approved these changes Jul 11, 2017

View reviewed changes

Introduce ArbitraryWorkaround object

8ac1b9a

johnynek reviewed Jul 12, 2017

View reviewed changes

Bump scalding version

b860fb9

pankajroark approved these changes Jul 22, 2017

View reviewed changes

ttim merged commit 0f06d22 into twitter:develop Jul 25, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support Scala 2.12 #724

Support Scala 2.12 #724

ttim commented Jun 2, 2017

ttim commented Jun 2, 2017

ttim commented Jul 11, 2017

ttim commented Jul 11, 2017

piyushnarang left a comment

piyushnarang Jul 11, 2017

johnynek Jul 12, 2017

johnynek commented Jul 12, 2017

ttim commented Jul 12, 2017 •

edited

Loading

non commented Jul 12, 2017

ttim commented Jul 12, 2017

non commented Jul 12, 2017

non commented Jul 12, 2017

ttim commented Jul 12, 2017

ttim commented Jul 12, 2017

johnynek Jul 12, 2017

ttim Jul 13, 2017

ttim Jul 21, 2017

johnynek left a comment

non commented Jul 13, 2017

ttim commented Jul 13, 2017

travisbrown-stripe commented Jul 13, 2017

ttim commented Jul 21, 2017

travisbrown-stripe commented Jul 21, 2017

codecov-io commented Jul 21, 2017

ttim commented Jul 21, 2017

Support Scala 2.12 #724

Support Scala 2.12 #724

Conversation

ttim commented Jun 2, 2017

ttim commented Jun 2, 2017

ttim commented Jul 11, 2017

ttim commented Jul 11, 2017

piyushnarang left a comment

Choose a reason for hiding this comment

piyushnarang Jul 11, 2017

Choose a reason for hiding this comment

johnynek Jul 12, 2017

Choose a reason for hiding this comment

johnynek commented Jul 12, 2017

ttim commented Jul 12, 2017 • edited Loading

non commented Jul 12, 2017

ttim commented Jul 12, 2017

non commented Jul 12, 2017

non commented Jul 12, 2017

ttim commented Jul 12, 2017

ttim commented Jul 12, 2017

johnynek Jul 12, 2017

Choose a reason for hiding this comment

ttim Jul 13, 2017

Choose a reason for hiding this comment

ttim Jul 21, 2017

Choose a reason for hiding this comment

johnynek left a comment

Choose a reason for hiding this comment

non commented Jul 13, 2017

ttim commented Jul 13, 2017

travisbrown-stripe commented Jul 13, 2017

ttim commented Jul 21, 2017

travisbrown-stripe commented Jul 21, 2017

codecov-io commented Jul 21, 2017

Codecov Report

ttim commented Jul 21, 2017

ttim commented Jul 12, 2017 •

edited

Loading