Scalding updates for Scala 2.12 #1646

piyushnarang · 2017-02-25T01:09:46Z

Based on @sritchie's PR (#1630) so that we can try and get this in and then cut the 0.17.0 release. Putting this up so that we can have travis catch any test failures (2.12 seems to compile ok on my machine, didn't run all tests though).

johnynek · 2017-02-25T19:27:19Z

Looks like some errors in the test package.

Thanks for pushing on this.

piyushnarang · 2017-02-27T18:10:20Z

@johnynek yeah was checking out some of the failures. Will update the PR with fixes and ping you / others when the build is all green.

piyushnarang · 2017-02-28T21:18:45Z

@johnynek / @isnotinvain / @pankajroark question for you folks. It looks like there's no version of the scrooge-sbt-plugin that supports both 2.10 and 2.12. Support for 2.10 has been dropped on their end a while back. Can we do the same as well? Drop support for 2.10?

johnynek · 2017-02-25T19:24:43Z

project/plugins.sbt

@@ -16,7 +16,6 @@ addSbtPlugin("com.typesafe"       % "sbt-mima-plugin"     % "0.1.12")
 addSbtPlugin("com.typesafe.sbt"   % "sbt-ghpages"         % "0.5.4")


mima 0.1.14 is also out with some bug fixes.

Ok, I'll pick it up as well.

johnynek · 2017-03-01T03:38:52Z

if it is not too expensive can we do the same 2.10 strategy we did in chill?

I am not opposed to dropping 2.10, I guess, but honestly, if it is pretty easy to support, maybe not.

That said, if you are on 2.10 you probably aren't updating much, and we are both on 2.11, so maybe 2.11 is fine?

piyushnarang · 2017-03-01T18:18:51Z

Yeah if we want to continue supporting 2.10, we'll have to drop the scalding-parquet and scalding-parquet-scrooge projects (as they / their tests depend on scrooge). I'm more in favor of dropping 2.10 support. This change is going to be part of a major release (0.17.0) so we can make some substantial updates (like dropping 2.10). 2.10 seems to have been EOLed around 2 years back (https://www.scala-lang.org/news/2016-schedule.html). It seems like we're going to keep running into issues from time to time if we continue to support it. Seems like it's not worth the support burden when we have 2.11.x and with this change 2.12.x.

johnynek · 2017-03-01T18:24:27Z

sounds good to me. Also testing is slow if we have 3 versions since our tests are already crazy slow.

piyushnarang · 2017-03-01T18:27:58Z

@johnynek yeah I initially thought that my travis build for this PR might have been down yesterday due to this 3 version explosion :-). But it turned out to be s3's fault.

johnynek · 2017-03-01T18:28:47Z

maybe you brought down s3?

;)

oscar-stripe · 2017-03-16T22:25:47Z

looks like this line fails in 2.12:
https://github.com/twitter/scalding/blob/develop/scalding-core/src/main/scala/com/twitter/scalding/LineNumber.scala#L75

we need to catch the exception for lambdas.

gkk-stripe · 2017-03-17T00:55:45Z

I found this problem with Box.scala when trying to upgrade scalding-core to Scala 2.12: https://issues.scala-lang.org/browse/SI-10232

The work-around is to remove half of boxes from Box.scala. Once I've done it, I got all tests in core to pass.

johnynek · 2017-03-17T00:56:12Z

Note that @gkk-stripe found this issue we are hitting with boxed:

https://issues.scala-lang.org/browse/SI-10232

johnynek · 2017-03-17T00:56:25Z

:) race...

piyushnarang · 2017-03-17T01:53:45Z

Ah thanks @gkk-stripe that's really cool. I was breaking my head against that one for a while :-). I'll try out your suggestion in the next couple of days (or if you want to push a commit on this PR so that you get credit for it, feel free to do so).

johnynek

Those may need to be on different objects and we may need to import. Can we make those objects private?

piyushnarang · 2017-03-17T03:07:03Z

@johnynek yeah I can try that. I did try running the scalding-core tests and they worked locally with these changes. Seems to be failing on travis though.

johnynek · 2017-03-17T03:08:00Z

Well, if it works it works.

…

On Thu, Mar 16, 2017 at 17:07 Piyush Narang ***@***.***> wrote: @johnynek <https://github.com/johnynek> yeah I can try that. I did try running the scalding-core tests and they worked locally with these changes. Seems to be failing on travis though. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#1646 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAEJdpc7sad8Yxe5D5sKZDujSlA5SKziks5rmfjYgaJpZM4ML2Ai> .

piyushnarang · 2017-03-17T21:32:05Z

Trying to figure out why the travis build is failing. Tried building a couple of the projects on my machine and they seem to succeed:
./sbt ++2.12.1 clean scalding-thrift-macros/test same with scalding-core/test.

johnynek · 2017-03-18T00:23:41Z

I am seeing:

  at com.twitter.scalding.LineNumber$.$anonfun$tryNonScaldingCaller$2(LineNumber.scala:75)
[info]   at com.twitter.scalding.LineNumber$.$anonfun$tryNonScaldingCaller$2$adapted(LineNumber.scala:74)
[info]   at com.twitter.scalding.LineNumber$$$Lambda$401/869703951.apply(Unknown Source)
[info]   at scala.collection.Iterator$$anon$12.hasNext(Iterator.scala:502)
[info]   at com.twitter.scalding.LineNumber$.headOption$1(LineNumber.scala:68)
[info]   at com.twitter.scalding.LineNumber$.tryNonScaldingCaller(LineNumber```

Which I mentioned as a problem above: 
https://github.com/twitter/scalding/pull/1646#issuecomment-287209798

we need to handle line numbers differently for 2.12 since lambdas might not be able to reflect like that?

piyushnarang · 2017-03-18T00:29:43Z

@johnynek yeah I pinged you on Gitter. So this seems to be an issue only on travis. I’ve tried a bunch of combinations of trying to repro this locally (directly calling ./sbt ++2.12.1 scalding-thrift-macros/test, invoking it via our run_test.sh script, switching to java 8_31 as thats what travis uses). None of those seem to trigger it. Not sure if I’m missing something there, it seems to consistently succeed on my machine (also got Timur to test on his machine).

piyushnarang · 2017-03-18T01:47:14Z

Debugging this locally, I don't see the 'com.twitter.scalding.typed.ReduceStep$$Lambda$275/727580562' showing up in the list of StackTraceElements passed to the method which is why it seems like I don't hit this error on my machine. Not sure why / how they're present on the travis CI build.

johnynek · 2017-03-18T02:08:41Z

Piyush, you are testing locally with 2.12? It would only happen with 2.12.

…

On Fri, Mar 17, 2017 at 15:47 Piyush Narang ***@***.***> wrote: Debugging this locally, I don't see the 'com.twitter.scalding.typed.ReduceStep$$Lambda$275/727580562' showing up in the list of StackTraceElements passed to the method which is why it seems like I don't hit this error on my machine. Not sure why / how they're present on the travis CI build. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#1646 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAEJdtX3fpqTRzLtILl-Qv7gp0z6GsViks5rmzejgaJpZM4ML2Ai> .

piyushnarang · 2017-03-20T17:41:50Z

@johnynek while debugging this in Intellij I updated my scalaVersion in the build.sbt file to 2.12.1. While trying to repro before I ran the commands above (and Timur also tried one set) on my command line.

johnynek · 2017-03-20T17:55:36Z

Well, i mean, in any case Class.forName might throw. We could put a try/catch and handle that in a decent way. Even if we can't repro, I can see how it may be a bug, I just don't know in what cases it is triggered. The only risk to such an approach is that the bug is elsewhere and we are hiding it with a fix like this. But if we can get the tests green locally and on Travis, that seems okay to me (if we don't have to change the tests to do it).

piyushnarang · 2017-03-20T18:15:31Z

@johnynek yeah I had the changes to catch the CNF handy but I was also worried about the masking of the issue elsewhere which is why I was trying out all those combinations to repro :-). I can push that upstream so that we can see if that makes Travis happy.

piyushnarang · 2017-03-21T01:07:00Z

@johnynek ok the build is green now :-). Do take a look when you get the time. I'll also sanity check this and clean up stuff when I get in tomorrow.

johnynek · 2017-03-21T03:06:15Z

build.sbt

 val cascadingAvroVersion = "2.1.2"
-val chillVersion = "0.8.3"
+val chillVersion = "0.8.4"


are we sure we want to bump to 0.8.x vs 0.7.7? Seems summingbird on storm 1.0 has still not landed, so we might want to have one last summingbird with 2.12? (I'm working a lot with summingbird and that may help me).

@johnynek I can switch this to 0.7.7. If I understand correctly the plan would be:

Merge this PR with chill 0.7.7 (might need to revert the test fix here: https://github.com/twitter/scalding/pull/1634/files).

Cut a 0.17.0 release

You can use that to cut a SB 2.12 rel on chill 0.7.x

Switch back to chill 0.8.4

Cut a 0.17.1 rel.

We can use this for our SB 2.12 rel on chill 0.8.x

@ttim - does this sound reasonable to you as well?

@johnynek summingbird with storm 1.0 support wasn't released because I'm waiting on scalding/storehaus to be released with 0.8.x chill.

In order to release SB with 0.9 storm and 2.12 scala support we also need to release tormenta with the same. Latest tormenta's release uses storm 1.0 (which is not good for above) and supports 2.12.

So I see two possibilities:

If we really want to release SB with 0.9 storm and 2.12 scala:

release old version of tormenta with 2.12 support only, without 1.0 storm

release scalding 0.17.0 as @piyushnarang said

release storehaus & SB with 2.12 & 0.9 storm.

And iterate again with scalding/storehaus/SB releases to get SB with 2.12 & 1.0 storm.

Or if we are ok without SB for 2.12 and 0.9 storm:

release scalding with 0.8.x chill & 2.12 support

release storehaus/SB with 2.12 & 1.0 storm

One more note - we use SB with 1.0 storm at Twitter for last couple of months and it works well. Transition to new api wasn't painful.

Okay, I think moving forward with 0.8 chill seems fine then. Let's move forward and go ahead to get on 1.0 storm.

johnynek · 2017-03-21T03:09:05Z

scalding-core/src/main/scala/com/twitter/scalding/Execution.scala

+  override def equals(other: Any): Boolean = {
+    //removed from Scala 2.12
+    def _equals(x: Product, y: Any): Boolean = y match {
+      case y: Product if x.productArity == y.productArity => x.productIterator sameElements y


can we wrap this line:

case y: Product if x.productArity == y.productArity => x.productIterator sameElements y.productIterator

sure, seems cleaner will do.

johnynek · 2017-03-21T03:10:42Z

scalding-core/src/main/scala/com/twitter/scalding/typed/Sketched.scala

+      .map { case (k, _) => ((), Batched(cms.create(Bytes(serialize(k))))) }
+      .sumByLocalKeys
+      .map {
+        case (_, batched) => batched.sum


this looks like some kind of bad merge. I don't think this is part of this. Can you rebase develop and see if this goes away?

johnynek · 2017-03-21T03:11:39Z

...adoop-test/src/test/scala/com/twitter/scalding/reducer_estimation/ReducerEstimatorTest.scala

@@ -106,6 +125,24 @@ class ReducerEstimatorTest extends WordSpec with Matchers with HadoopSharedPlatf
        .run()
    }

+    "run with correct number of reducers when we have a glob pattern in path" in {


this looks like another merge issue.

johnynek · 2017-03-21T03:11:55Z

scalding-repl/src/main/scala/com/twitter/scalding/ScaldingShell.scala

@@ -124,7 +124,7 @@ trait BaseScaldingShell extends MainGenericRunner {
   * @param args from the command line.
   */
  def main(args: Array[String]): Unit = {
-    val retVal = process(args)
+    val retVal = process(ExpandLibJarsGlobs(args))


another merge issue.

oscar-stripe · 2017-03-21T23:55:03Z

scalding-parquet-scrooge-fixtures/src/test/resources/test.thrift

@@ -86,14 +86,6 @@ struct StructWithReorderedOptionalFields {
  1: optional i32 fieldOne,
 }

-struct TestPersonWithRequiredPhone {


why did this need to get nuked?

The scrooge generator was cribbing cause this was being declared twice. The old version seem to not mind.

In this file: https://github.com/twitter/scalding/blob/7ed0f92a946ad8407645695d3def62324f78ac41/scalding-parquet-scrooge-fixtures/src/test/resources/test.thrift it's on 89 and 107

johnynek · 2017-03-22T18:45:51Z

👍

johnynek · 2017-03-22T18:46:08Z

(when we merge, please squash merge this).

johnynek · 2017-03-22T18:46:24Z

(which github's merge button supports).

ttim

Minor comment

ttim · 2017-03-22T18:54:16Z

scalding-core/src/main/scala/com/twitter/scalding/Execution.scala

+
+  override def equals(other: Any): Boolean = {
+    //removed from Scala 2.12
+    def _equals(x: Product, y: Any): Boolean = y match {


I prefer to have this method inlined and without the comment.

Cool, will do.

fwbrasil · 2017-03-22T18:43:19Z

build.sbt

 val slf4jVersion = "1.6.6"
 val thriftVersion = "0.5.0"
 val junitVersion = "4.10"
 val macroCompatVersion = "1.1.1"
+val jlineVersion = "2.14.3"
+
+def scrooge(scalaVersion: String) = "com.twitter" %% "scrooge-serializer" % scroogeVersion


What's the purpose of this method? It doesn't use the scalaVersion parameter, maybe it should be a val?

I think that was old stuff from when we were still supporting scala 2.10 (which scrooge stopped supporting).

yeah I can drop this. Didn't clean it up after a couple of iterations.

fwbrasil · 2017-03-22T20:10:51Z

scalding-serialization/src/main/scala/com/twitter/scalding/serialization/Boxed.scala

@@ -560,7 +562,7 @@ object Boxed {
   * using `Any` here instead of parametrizing everything over a type parameter
   * `K`.
   */
-  private[this] val allBoxes = List(
+  val boxes1 = List(


could these lists be private[serialization]?

+1 to as private as possible. Good catch.

ok, let me try that.

piyushnarang · 2017-03-22T23:01:15Z

@ttim / @fwbrasil - updated with your comments

ttim · 2017-03-23T00:42:16Z

scalding-core/src/main/scala/com/twitter/scalding/Execution.scala

-      case y: Product if x.productArity == y.productArity =>
-        x.productIterator sameElements y.productIterator
+    val productEquals = other match {
+      case p: Product if self.productArity == p.productArity =>


nit: I guess hashCode check was made before _equals call for perf reasons. Therefore this code has slightly worse performance if it matters.

I recommend just remove separate hashCode == other.hashCode check and put it into if part.

Updated this. Not sure how large those products get. I moved the hashCode into the case body.

fwbrasil · 2017-03-23T01:44:25Z

👍 LGTM

piyushnarang mentioned this pull request Feb 27, 2017

Request for Scalding release 0.17.0 #1641

Merged

johnynek reviewed Mar 1, 2017

View reviewed changes

johnynek reviewed Mar 17, 2017

View reviewed changes

johnynek requested changes Mar 21, 2017

View reviewed changes

sritchie-stripe and others added 2 commits March 21, 2017 10:20

start upgrade to 2.12

1c85a3a

Bump bijection, chill

aff0070

Piyush Narang added 13 commits March 21, 2017 10:22

Fix scalding-db tests

97e8df0

Exclude guava from scrooge imports

feb5545

Use mima 0.1.14

12280e0

Update scalding-thrift-macros jdk version to java8

8cfe4db

Fix OrderedSerialization test failures

6db5cf2

Break boxed lambdas into 2 lists to get around Scala 2.12 bug

3b2bef7

Try catching CNF in LineNumber

0c4033b

Drop line numbers from hashJoin scalding hadoop test

4459b80

Switch default to jdk8 as scrooge deps have been built with that

60ef980

Update scald.rb to pick up 2.12

470d85f

Try latest jline version

b3a40e6

Try and fix merge issues

ed55f8a

Wrap Execution line better

643220f

piyushnarang force-pushed the pnarang/scala212wip branch from a77be5e to 643220f Compare March 21, 2017 17:43

oscar-stripe reviewed Mar 21, 2017

View reviewed changes

johnynek approved these changes Mar 22, 2017

View reviewed changes

ttim approved these changes Mar 22, 2017

View reviewed changes

fwbrasil reviewed Mar 22, 2017

View reviewed changes

Piyush Narang added 3 commits March 22, 2017 14:41

Remove scrooge method

36ca140

Drop _equals method

5201750

Try private[ser] for Boxed issues

26237d6

ttim reviewed Mar 23, 2017

View reviewed changes

Reorder hashCode and equals

f2fd0f2

piyushnarang merged commit 12aaa7e into twitter:develop Mar 23, 2017

johnynek mentioned this pull request Jun 15, 2017

{WIP} Upgrade to 2.12 #1630

Closed

2 tasks

		@@ -16,7 +16,6 @@ addSbtPlugin("com.typesafe" % "sbt-mima-plugin" % "0.1.12")
		addSbtPlugin("com.typesafe.sbt" % "sbt-ghpages" % "0.5.4")

Scalding updates for Scala 2.12 #1646

Scalding updates for Scala 2.12 #1646

Conversation

piyushnarang commented Feb 25, 2017

johnynek commented Feb 25, 2017

piyushnarang commented Feb 27, 2017

piyushnarang commented Feb 28, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

johnynek commented Mar 1, 2017

piyushnarang commented Mar 1, 2017

johnynek commented Mar 1, 2017

piyushnarang commented Mar 1, 2017

johnynek commented Mar 1, 2017

oscar-stripe commented Mar 16, 2017

gkk-stripe commented Mar 17, 2017 • edited Loading

johnynek commented Mar 17, 2017

johnynek commented Mar 17, 2017

piyushnarang commented Mar 17, 2017

johnynek left a comment

Choose a reason for hiding this comment

piyushnarang commented Mar 17, 2017

johnynek commented Mar 17, 2017 via email

piyushnarang commented Mar 17, 2017

johnynek commented Mar 18, 2017

piyushnarang commented Mar 18, 2017

piyushnarang commented Mar 18, 2017

johnynek commented Mar 18, 2017 via email

piyushnarang commented Mar 20, 2017

johnynek commented Mar 20, 2017

piyushnarang commented Mar 20, 2017

piyushnarang commented Mar 21, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

johnynek commented Mar 22, 2017

johnynek commented Mar 22, 2017

johnynek commented Mar 22, 2017

ttim left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

piyushnarang commented Mar 22, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fwbrasil commented Mar 23, 2017

gkk-stripe commented Mar 17, 2017 •

edited

Loading