search

twitter / finatra

Fast, testable, Scala services built on TwitterServer and Finagle
https://twitter.github.io/finatra/
Apache License 2.0
2.27k stars 406 forks source link

Cross-build Scala 2.13.0 #522

Closed yufangong closed 3 years ago

yufangong commented 4 years ago

Scala 2.13 is released and finagle and twitter-server is close to finishing cross-building 2.13. It would be great to have finatra prepare for enabling cross-building 2.13, and @blueway first opened an issue to ask for 2.13 support. We want to do a similar fashion to https://github.com/twitter/finagle/issues/771.

Library dependencies: Twitter-Server, Finagle

Subprojects: They are ordered by dependency depth, the first layer (httpAnnotations and injectSlf4j) should have the least dependencies. Note each subproject may have a TestJarSources with them that can be upgraded at the same time (for example: httpclient and httpclientTestJarSources.

If you start working on one of these subprojects, please add a comment on this thread and I'll add your name next to it so people know not to duplicate your work.

chrisbenincasa commented 4 years ago

I believe all of Finagle is properly cross built for 2.13 now. I think I should have some time to help out here. I've just opened 3 dependent PRs:

I can start taking a look at injectCore as well too, to help unblock other submodules.

EDIT: I've also opened up PRs for inject-core (#530), inject-utils (#532), inject-modules (#531), and inject-ports (#533).

chrisbenincasa commented 4 years ago

Also of note: some core test library dependencies must have their versions bumped in order to make the entire repo eligible for cross-building to 2.13. It seems like this will be the biggest hurdle since I imagine that these versions must be updated on the internal monorepo version. However, until those versions are updated (see #527), it seems that none of the other cross-building changes will be able to be merged in.

yufangong commented 4 years ago

@chrisbenincasa Thanks a lot! We will soon get them reviewed!

ihasfrozen commented 4 years ago

I'd be happy to take a shot at validation if it's next in line!

cacoco commented 4 years ago

Note that we use net.codingwell#scala-guice and we are currently on version 4.2.0. This is a central library and we need to get to at least version 4.2.5 which supports Scala 2.13. Unfortunately the library switches from using Manifests to TypeTags for building Guice Key types from a TypeLiteral which is a breaking change for Finatra. We are currently working on upgrading this library (along with the other libraries which need to move in order to be able to cross compile for Scala 2.13). Thanks!

chrisbenincasa commented 4 years ago

@cacoco thanks for the update!

chrisbenincasa commented 4 years ago

I think validation will have to moved to the utils level, since validation seems to depend on utils.

Additionally, jackson depends on both validation and utils so I believe that'll have to be regrouped as well.

yufangong commented 4 years ago

I think validation will have to moved to the utils level, since validation seems to depend on utils.

Additionally, jackson depends on both validation and utils so I believe that'll have to be regrouped as well.

Updated accordingly, thanks!

chrisbenincasa commented 4 years ago

I'm starting to look at converting some of the heavier modules (where performance is likely a concern) and am bumping up against the concerns posed in https://github.com/twitter/finagle/issues/818 . There is a lot of usages of ArrayBuffer and other mutable Seqs that have breaking changes in 2.13 and where the recommended solutions could have a performance impact. I'm wondering if any thought has gone into this internally

chrisbenincasa commented 4 years ago

@yufangong I'm hitting an unusual issue while attempting to convert http to the new version. Here is a gist of the compiler output:

https://gist.github.com/chrisbenincasa/ae5f6816d7c0394792c1607c1d208730

What's interesting is that javax.servlet is no longer a dependency on the project (I see it was removed over a year ago). http is compiling just fine on 2.11 and 2.12 with no changes. I've been digging around to see why 2.13 would affect this (and have double-checked various things, like my Java version, etc). Do you have any ideas?

chrisbenincasa commented 4 years ago

I was able to track this down a bit more... it seems like there is a transitive dependency on javax.servlet-api from apache commons, which is used here: https://github.com/twitter/finatra/blob/39fd964050e41982e7d360d0247847b8e04069d9/http/src/main/scala/com/twitter/finatra/http/fileupload/FinagleRequestFileUpload.scala#L8

This dep is marked as provided in apache commons-fileupload: https://github.com/apache/commons-fileupload/blob/commons-fileupload-1.4/pom.xml#L234-L239

I'm unclear on why this ends up throwing when compiling 2.13 only, however.

yufangong commented 4 years ago

@chrisbenincasa That's odd, I have no clue... I found a similar issue at https://github.com/sbt/sbt/issues/2958, it might be worth trying the suggested workarounds/tests about coursier/clean-up jars and consider filing an sbt issue if it is reproduced consistently.

chrisbenincasa commented 4 years ago

@yufangong interestingly, bumping from 2.13.1 to 2.13.2 resolves the problem. Perhaps we should target that version?

yufangong commented 4 years ago

I'm starting to look at converting some of the heavier modules (where performance is likely a concern) and am bumping up against the concerns posed in twitter/finagle#818 . There is a lot of usages of ArrayBuffer and other mutable Seqs that have breaking changes in 2.13 and where the recommended solutions could have a performance impact. I'm wondering if any thought has gone into this internally

Thanks for bringing this up! At this point, 2.12 performance has a more significant impact, so I think compromising 2.13 perf is acceptable. Moving forward, we will evaluate the plausible solutions and any other proposed solution for both 2.12/2.13. There are a few benchmarks sitting in finatra/benchmarks for http/jackson/validation that could be leveraged.

cacoco commented 4 years ago

@chrisbenincasa RE: javax.servlet-api from apache commons being marked as provided in apache commons-fileupload: https://github.com/apache/commons-fileupload/blob/commons-fileupload-1.4/pom.xml#L234-L239. It is confusing why this would work at all with sby/ivy. We should likely declare the dependency explicitly in finatra/http for correctness. Provided is intended to signal that it is required to compile but that you need to bring this dependency (or a binary compatible version) yourself -- it was likely a mistake for us to remove the javax.servlet-api dependency in this case.

mosesn commented 3 years ago

@chrisbenincasa just a heads up since your name is next to "utils"–someone is looking at "utils" internally too, have you already tackled it? Might be good to hold off if you haven't started yet

chrisbenincasa commented 3 years ago

@mosesn yes, #535 converts utils

mosesn commented 3 years ago

@chrisbenincasa ack, we'll work something out so that the two diffs don't collide.

chrisbenincasa commented 3 years ago

@mosesn sounds good. FWIW, the change there simply adds the cross version. Not sure if the internal changes are meant as rewrites to use features from 2.13, but there shouldn't be any overlap since I'm simply trying to get versions published for 2.13 for immediate use.

sideshowcoder commented 3 years ago

Hey @chrisbenincasa I'm making the same change to utils as #535 to get the cross build, plus resolving some warnings which came with 2.13, the line in build.sbt is the same and as far as I can see #535 only makes that change to utils, so I don't think there should be a conflict there.

chrisbenincasa commented 3 years ago

Sounds good!

sideshowcoder commented 3 years ago

Took a look through the build.sbt to check what the current dependencies look like, maybe helpful for others looking into this. If people don't mind I would tackle kafkaStreamsQueryableThriftClient next as it is currently not blocked by anything.

Unblocked

Blocked

yufangong commented 3 years ago

Thank you @sideshowcoder! You got it.

chrisbenincasa commented 3 years ago

Awesome!

I started looking at Kafka, this may be another dependency issue. It seems the 2.4.0 release was the first to support Scala 2.13. Finatra is currently on 2.2.0. Do we have to wait for an internal repo version bump?

yufangong commented 3 years ago

Awesome!

I started looking at Kafka, this may be another dependency issue. It seems the 2.4.0 release was the first to support Scala 2.13. Finatra is currently on 2.2.0. Do we have to wait for an internal repo version bump?

It looks like some internal work started to upgrade kafka: https://github.com/twitter/finatra/commit/3c78c34df0c55f3b5dec9717d54749a3f5dc751e, a skim of that I assume we will need to have scala 2.12/2.13 diverged in kafka projects, I will follow up internally and let you know. Thanks!

sideshowcoder commented 3 years ago

injectDtab was missing from the list as I think it was created after this issue was first opened, it builds fine so I'm going add the cross build setting.

mosesn commented 3 years ago

Thanks @sideshowcoder! added it to the list at the top

sideshowcoder commented 3 years ago

@yufangong I had a look and it looks like injectThriftClient, injectLogback, httpMustache, injectThriftClientHttpMapper, and benchmarks all build fine agains 2.13.1. There are a bunch of deprecation warnings so which are probably worth fixing. Mind if I take those on? I see that kafka is already worked on.

yufangong commented 3 years ago

Nice! Thank you @sideshowcoder. Added to the items.

chrisbenincasa commented 3 years ago

@yufangong do you have an update on the internal work with Kafka? Wondering if the remaining modules listed above are ready to be converted or not.

yufangong commented 3 years ago

@chrisbenincasa Sorry! I haven't look into it. I'm planning to allocate sometime next week to evaluate the work.

yufangong commented 3 years ago

Hey @chrisbenincasa, I have some updates:

The finatra/kafka targets are a bit complicated regards build, after talking with the supporting team we think currently the most efficient way is to fork the scala 2.13/2.12 on the finatra-kafka-stream projects. Forcing 2.13 to use kafka 2.5 and 2.12/2.11 to use kafka 2.2. kafka 2.5 should have all things build up equally supported with 2.2 but as beta, I suspect some hiccups can occur in tests.

The directories are already split into kafka-2.2 and kafka-2.5 within the related packages.

We should probably weigh in more opinions/suggestions before totally settle with this solution, in the meantime, feel free to poke around, first-hand experience will be great!

mucahitkantepe commented 3 years ago

If it was a blocker, Kafka is published for Scala 2.13 via 2.7.0

cacoco commented 3 years ago

Thanks to everyone for your help (especially @yufangong, @chrisbenincasa, and @sideshowcoder) we're now fully cross-building with Scala 2.13 as of 7deb11535e5c9eb0787326e64dc44bf060b935df! ✅

The upcoming February release should be cross-published with Scala 2.13.

chrisbenincasa commented 3 years ago

Woohoo!

cacoco commented 3 years ago

2.13 artifacts now published with release 21.2.0: https://repo1.maven.org/maven2/com/twitter/

sideshowcoder commented 3 years ago

AWESOME! 🎉