twitter / finagle

A fault tolerant, protocol-agnostic RPC system
https://twitter.github.io/finagle
Apache License 2.0
8.79k stars 1.46k forks source link

finagle-*: scala 2.11 compatibility #290

Closed mosesn closed 9 years ago

mosesn commented 10 years ago

It looks like several people are interested in converting finagle over to scalatest. Since finagle has many small projects, and @p-antoine has already done the biggest chunk, finagle-core, I'm optimistic that we can be done with this quickly.

coordination

Here are the finagle-benchmark :heavy_check_mark: finagle-commons-stats :heavy_check_mark: finagle-core :heavy_check_mark: finagle-example :heavy_check_mark: finagle-exception :heavy_check_mark: finagle-exp :heavy_check_mark: finagle-http :heavy_check_mark: @adamdecaf finagle-kestrel :o: @chester89 finagle-mdns :heavy_check_mark: finagle-memcached :heavy_check_mark: @rlazoti finagle-mux :heavy_check_mark: finagle-mysql :heavy_check_mark: finagle-native :heavy_check_mark: @adamdecaf finagle-ostrich4 :heavy_check_mark: finagle-protobuf :heavy_check_mark: finagle-redis :o: @penland365 finagle-serversets :heavy_check_mark: finagle-spdy :heavy_check_mark: finagle-stats :heavy_check_mark: finagle-stream :o: @bajohns finagle-stress :heavy_check_mark: finagle-swift :heavy_check_mark: finagle-test :heavy_check_mark: finagle-testers :heavy_check_mark: finagle-thrift :o: @bajohns finagle-thriftmux :heavy_check_mark: finagle-validate :heavy_check_mark: finagle-zipkin :heavy_check_mark:

If we could coordinate our efforts, that would be super awesome.

@penland365 has already staked a claim to finagle-redis. If there's a project that you want to work on, please post on this thread!

If someone is working on something you want to help with, please get in contact with them, make sure they're OK with you helping out, and then get in contact with us.

tips

  1. Follow the CONTRIBUTING guidelines.
  2. Make sure your test works with JUnit (see other scalatest tests to see how to go about doing it).
  3. Test with 2.9.2, 2.10.x, and 2.11.x. They need to all work for your submission to be accepted.
  4. Make sure your scalatest versions are lined up properly. It seems like scalatest has stopped making new versions for 2.9.2, and the earliest scalatest_2.11 is after that cutoff, so there will have to be two scalatests.
  5. Remove the specs dependency from your project to be 100% sure that you're no longer depending on it.
  6. Don't touch the maven or pants files, we'll do it on our end.
  7. Please rename scalatest-ified classes and file names from XSpec to XTest
  8. For mocking, please use org.scalatest.mock.MockitoSugar.
  9. If you have a question about something, look around the rest of the repo to see how it's done. If you still have questions, reach out on this thread.
adamdecaf commented 10 years ago

I can grab finagle-http.

mosesn commented 10 years ago

It's yours!

bajohns commented 10 years ago

I can take finagle-thrift and finagle-stream.

mosesn commented 10 years ago

Excellent! Noted.

rlazoti commented 10 years ago

is finagle-memcached available? If it's, I can take it.

mosesn commented 10 years ago

For @rlazoti, anything!

chester89 commented 10 years ago

Can I take a shot at finagle-kestrel?

mosesn commented 10 years ago

Boom! Only finagle-native is left now.

adamdecaf commented 10 years ago

Looks easy enough. I got it.

mosesn commented 10 years ago

And all gone. Thanks!

mosesn commented 10 years ago

Forgot to mention before, if you're working with mocks, please use MockitoSugar. I've updated the list of guidelines to reflect this too.

chester89 commented 10 years ago

What should I do with tests marked as deprecated? Should I convert them too?

mosesn commented 10 years ago

Yeah, let's convert them too. We can remove it after we've actually deleted it.

caniszczyk commented 10 years ago

@mosesn any idea what's left with this one?

mosesn commented 10 years ago

@caniszczyk I've been keeping the description of the issue up to date. We need to review @bajohns' PRs, and then the only ones that are left are finagle-redis (mostly done) and finagle-kestrel (unsure of status).

penland365 commented 10 years ago

I hope to have finagle-redis finished up by the end of the weekend - Monday. My apologies if this is holding anyone up!

mosesn commented 10 years ago

It's not--we still have more work we can do on our end, don't worry about it :panda_face:

chester89 commented 10 years ago

Guys, finagle-kestrel is on me, I apologise for not reporting for so long. What I managed to do is to port 99% of the code to ScalaTest, and it does compile - but many tests fail because I somehow screwed up asserts on collections. Let me post the code I have tomorrow - and may be someone can help me finish it.

mosesn commented 10 years ago

Sounds like a plan! Thanks for following up.

chester89 commented 10 years ago

Send a PR with what I've got.

chester89 commented 10 years ago

great! will do

2014-08-15 18:29 GMT+04:00 Pierre-Antoine Ganaye notifications@github.com:

@chester89 https://github.com/chester89 I've converted all tests except ReadHandleSpec take a look https://github.com/p-antoine/finagle/tree/master/finagle-kestrel/src/test/scala/com/twitter/finagle/kestrel everything works.

— Reply to this email directly or view it on GitHub https://github.com/twitter/finagle/issues/290#issuecomment-52311801.

С уважением, Чермённов Глеб, тел. (916) 314-9324

rlazoti commented 10 years ago

I am also available to help anyone in a pinch! :smiley:

mosesn commented 10 years ago

Actually, if you folks are rip-roaring to go, it would be great to get started on scrooge and ostrich, which are both dependencies of finagle. The other thing that needs to be done is to actually turn on 2.11 support for finagle. We can't do it properly until we're totally off of specs, but we can start off by just making a branch where we remove the specs bits and make sure everything else compiles, and all of our dependencies are OK.

Any interest?

rlazoti commented 10 years ago

@mosesn I can take ostrich project. :wink:

mosesn commented 10 years ago

Oh hey, you're right! Looks like @dhelder removed specs from scrooge already, nice!

For scrooge, we need to update dependencies, but finagle is one of the dependencies (finagle depends on scrooge-core, scrooge-runtime depends on finagle, it's a mess but luckily not a cyclic dependency).

We'll also need to update scrooge-core to cross-publish against 2.11, and it would be great to rename all of the Spec.scala files to Test.scala, and similarly rename the tests.

elbiczel commented 10 years ago

I'll start working on scrooge-core early next week. Is there a separate issue for scrooge?

2014-08-15 17:39 GMT+02:00 Moses Nakamura notifications@github.com:

Oh hey, you're right! Looks like @dhelder https://github.com/dhelder removed specs from scrooge already, nice!

For scrooge, we need to update dependencies, but finagle is one of the dependencies (finagle depends on scrooge-core, scrooge-runtime depends on finagle, it's a mess but luckily not a cyclic dependency).

We'll also need to update scrooge-core to cross-publish against 2.11, and it would be great to rename all of the Spec.scala files to Test.scala, and similarly rename the tests.

— Reply to this email directly or view it on GitHub https://github.com/twitter/finagle/issues/290#issuecomment-52320210.

mosesn commented 10 years ago

Rad! Happy to help if you run into any issues.

rlazoti commented 10 years ago

Guys, I am working on ostrich repo and I found a completely commented test (https://github.com/twitter/ostrich/blob/master/src/test/scala/com/twitter/ostrich/stats/JsonStatsFetcherSpec.scala). Should I convert it or I could just delete that test?

mosesn commented 10 years ago

My instinct is delete it, but file an issue to test that script properly (or else delete it). @mariusaeriksen looks like you were the one to remove it, do you have an opinion either way?

chester89 commented 10 years ago

@p-antoine just to clarify - as you've ported the code, you're making the PR later, right?

chester89 commented 10 years ago

I'd prefer you finish it, since you're MUCH closer to the result

mosesn commented 10 years ago

Ahh, that's a good point. I'll look into it, thanks for the heads up.

mosesn commented 10 years ago

OK, as far as I can tell, the dependency on util-eval is necessary for now. I'll see if I can deprecate it, but the fastest we'll be able to get off will be with a major release.

On the other hand, scala-json is not necessary. We can probably replace it with jackson + scala sugar.

In the mean time, we don't need to publish finagle-ostrich4 against 2.11, and only finagle-example and finagle-stress depend on finagle-ostrich4, so I think we'll be OK.

mosesn commented 10 years ago

@rlazoti can you tackle removing the dependency on scala-json? Or are you already busy enough with the specs => scalatest work?

rlazoti commented 10 years ago

@mosesn sure, I can do that! I'm going to finish the specs to scalatest conversion tomorrow.

mosesn commented 10 years ago

Rad! Very exciting. I'll see what we can do about deprecating the bits that depend on util-eval.

mosesn commented 10 years ago

@rlazoti https://github.com/twitter/twitter-server/blob/master/src/main/scala/com/twitter/server/util/JsonConverter.scala#L9 might be useful for inspiration.

rlazoti commented 10 years ago

Thanks @p-antoine! I'm going to add your updates into my branch. :+1: Should I send a PR with 6.20.0 util-eval's version even that it won't compile?

mosesn commented 10 years ago

Yeah, I agree with @p-antoine. It will be easier to merge this in piecemeal anyway. :musical_score:

WamBamBoozle commented 10 years ago

deprecating the bits that depend on util-eval

will util-eval be deprecated? Will it not be supported in scala 2.11?

On Sat, Aug 23, 2014 at 10:02 PM, Moses Nakamura notifications@github.com wrote:

Rad! Very exciting. I'll see what we can do about deprecating the bits that depend on util-eval.

— Reply to this email directly or view it on GitHub https://github.com/twitter/finagle/issues/290#issuecomment-53178603.

rlazoti commented 10 years ago

@p-antoine yeap! you are right. :) I am currently working to replace scala-json with jackson.

mauricio commented 10 years ago

Hello everyone, I'm unsure of what's missing here? Which projects still need someone to pick them up?

mosesn commented 10 years ago

@mauricio all of the finagle pieces are owned right now, but if you want to help contribute, twitter-server could use some love, just to check that all the tests pass with 2.11.

mosesn commented 10 years ago

@p-antoine I'm planning on deprecating the bits that remote it--I have a review up internally to do it, but still need to do a couple tests before removing it. Frankly, it's not going to happen super quickly. I think for the short term, the best plan will be to not build projects that depend on ostrich against 2.11.

alexflav23 commented 10 years ago

@bajohns @mosesn Any news or upgrades? We would happily offer more man power as we have a whole family of products, all running atop Finagle, that are waiting for this release.

bajohns commented 10 years ago

Hey @alexflav23 - my changes were merged and released publicly Oct 15th.

Thanks again to @mosesn, @stevegury, @bmdhacks and @travisbrown for moving the changes through.

sscarduzio commented 10 years ago

+1

rlazoti commented 10 years ago

@p-antoine, you're right. :) I think my work on ostrich is done. Please, let me know if there's something more I can do to help you with this issue.

dnatic09 commented 10 years ago

Great news and work by the contributors. I look forward to using Ostrich in 2.11.

mosesn commented 10 years ago

I think this is on me right now, I need to test out my ostrich removing util-eval change. I think @travisbrown is working on publishing the bits of finagle which don't depend on ostrich for 2.11, but not sure how that's progressing.