Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Move Scalding to 2.10/2.11 #1059

Closed
wants to merge 12 commits into from
Closed

Move Scalding to 2.10/2.11 #1059

wants to merge 12 commits into from

Conversation

jcoveney
Copy link
Contributor

So this is just the beginning, but it's a big chunk of the work. I've been doing it between other things.

The big things I've done are to get scalding-core and scalding-hadoop-test working. Note: the DistributedCache tests in scalding-core had a dependency on mockito.... for now I commented it out. Do we want to depend on mockito just for one test? That said, rewriting is going to be a slight pain. So I'm not sure what we want to do there.

Either way, this is to get the ball rolling. I haven't started the 2.11 work becasue we need a new algebird release...but the scalatest migration stuff is enough for the time being!

@@ -39,17 +39,17 @@ abstract class DailySuffixLzoCodec[T](prefix: String, dateRange: DateRange)(impl

abstract class DailySuffixLzoProtobuf[T <: Message: Manifest](prefix: String, dateRange: DateRange)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you kill the manifest references too while your at it?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was hoping to enlist the compiler in helping with this... can't do that until we're on 2.11 (ie algebird release). see: twitter/algebird#351

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How would the compiler help? if you enable warnings manifests are a warning in 2.10 also?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

By giving me errors instead of warnings ;)

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Manifests are still in 2.11 as a warning i'm pretty sure?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, really? well if that's the case, I'd prefer not to tackle that in this PR. This is going to get bit enough as it is and I'd like to focus on reconciling the breaking changes

@jcoveney
Copy link
Contributor Author

scalatest conversion is complete (ex. the Distributed Cache test I mentioned earlier, still not sure what to do about what)

Can work on 2.11 support once the new algebird is published

@jcoveney
Copy link
Contributor Author

Repl works! I searched around github and use https://github.com/NICTA/scoobi/blob/master/src/main/scala/com/nicta/scoobi/application/ScoobiRepl.scala as a base, as they had to deal with the same issue

@johnynek
Copy link
Collaborator

If we can get this green we can go ahead and merge before 0.12. I was mistaken about how quickly twitter would move to 0.12 and it is going to be a while. I'd rather go ahead and get this in sooner so that we don't block the scala 2.11 publishing.

@jcoveney
Copy link
Contributor Author

Everything works locally, I think that the build now takes a touch too long
and so travis kills it. Travis has a heartbeating mechanism but I'm not
sure if it works with how we have done things... we could turn logging back
on for assembly, but of course that creates a lot of noise.

@jcoveney
Copy link
Contributor Author

Hm, so it looks like the remaining errors are because of this:

[warn] Error extracting zip entry 'com/twitter/bijection/GeneratedTupleCollectionInjections$$anon$31$$anonfun$invert$10$$anonfun$apply$46$$anonfun$apply$47$$anonfun$apply$48$$anonfun$apply$49$$anonfun$apply$50$$anonfun$apply$51$$anonfun$apply$52$$anonfun$apply$53$$anonfun$apply$54$$anonfun$apply$55.class' to '/home/travis/build/twitter/scalding/scalding-core/target/streams/$global/assemblyOption/$global/streams/assembly/d05f13984ea9fe24547dacc7f0b063cebcb1a8a2_63edacff6d2b9004073350eb8ece9a976b686a82/com/twitter/bijection/GeneratedTupleCollectionInjections$$anon$31$$anonfun$invert$10$$anonfun$apply$46$$anonfun$apply$47$$anonfun$apply$48$$anonfun$apply$49$$anonfun$apply$50$$anonfun$apply$51$$anonfun$apply$52$$anonfun$apply$53$$anonfun$apply$54$$anonfun$apply$55.class': java.io.FileNotFoundException: /home/travis/build/twitter/scalding/scalding-core/target/streams/$global/assemblyOption/$global/streams/assembly/d05f13984ea9fe24547dacc7f0b063cebcb1a8a2_63edacff6d2b9004073350eb8ece9a976b686a82/com/twitter/bijection/GeneratedTupleCollectionInjections$$anon$31$$anonfun$invert$10$$anonfun$apply$46$$anonfun$apply$47$$anonfun$apply$48$$anonfun$apply$49$$anonfun$apply$50$$anonfun$apply$51$$anonfun$apply$52$$anonfun$apply$53$$anonfun$apply$54$$anonfun$apply$55.class (File name too long)

Have we dealt with problems like this before? This does seem like a pretty sillily long class name, but I'm not sure, really, of the best way to fix it (I guess we can try to unnest some callbacks?).

@jcoveney
Copy link
Contributor Author

So ian fixed scald.rb to work (the thing I was looking at was a red herring). It looks like we just have one failure left, a real one... it's the scalding repl again. It seems like on 2.11.2 if you run ./sbt clean; ./sbt ++2.11.2 "scalding-repl/run --local" it will fail when you try to do something. If you close out then run again, it works, leading me to believe something in the initialization and bootstrapping of the classpath has changed. I will try and debug this in my spare cycles... for reference, the implementations of MainGenericRunner are here:

https://github.com/scala/scala/blob/2.11.x/src/repl/scala/tools/nsc/MainGenericRunner.scala
https://github.com/scala/scala/blob/2.10.x/src/compiler/scala/tools/nsc/MainGenericRunner.scala

we override process() so it's easy to imagine we just need to account for a difference in the lifecyle.

We really should push on the scala folk to have a stable repl API... @travisbrown do you know why this API isn't treated as, well, an API? I know it's internal but I thought extending the REPL was considered a feature of scala, not an antipattern?

@johnynek johnynek mentioned this pull request Dec 5, 2014
@johnynek
Copy link
Collaborator

johnynek commented Dec 7, 2014

@ianoc can this be closed now?

@ianoc
Copy link
Collaborator

ianoc commented Dec 8, 2014

I think so, though Jco's comments above are pretty good to keep alive maybe in an issue until we have repl support working in 2.11

@johnynek
Copy link
Collaborator

This is done now, so closing this.

@johnynek johnynek closed this Feb 25, 2015
@johnynek johnynek deleted the jco/scalding_210 branch February 25, 2015 23:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants