xerial commented 5 years ago

Problem

We are using ~Finagle (via TwitterServer)~ Finatra with Presto 306, which uses Google Guice 4.2.2
TwitterServer depends on finatra-http -> inject-core -> scala_guice (4.0) -> Guice 4.0. This causes runtime binding exception when starting a Presto inside a project that uses Finagle.

Solution

Upgrading to the latest Guice 4.2.2 will resolve this Guice version conflict.
scala_guice also needs to be upgraded to 4.2.3, which depends on Guice 4.2.2.

Result

Guice 4.2.2 supports Java 11 https://github.com/google/guice/wiki/Guice422, so this change will be necessary anyway in near future.
scala-guice requires TypeTag to resolve typeLiteral[T]. Several interfaces need to be updated to pass TypeTag. If no runtimeClass is required, we can just pass TypeTag[T] instead of Manifest[T].

CLAassistant commented 5 years ago

All committers have signed the CLA.

codecov[bot] commented 5 years ago

Codecov Report

:exclamation: No coverage uploaded for pull request base (develop@8200671). Click here to learn what that means. The diff coverage is 87.5%.

@@            Coverage Diff             @@
##             develop     #491   +/-   ##
==========================================
  Coverage           ?   92.56%           
==========================================
  Files              ?      242           
  Lines              ?     3900           
  Branches           ?      293           
==========================================
  Hits               ?     3610           
  Misses             ?      290           
  Partials           ?        0

Impacted Files	Coverage Δ
.../main/scala/com/twitter/inject/TwitterModule.scala	`89.18% <ø> (ø)`
...n/scala/com/twitter/inject/TwitterBaseModule.scala	`100% <ø> (ø)`
...ter/finatra/http/exceptions/ExceptionManager.scala	`94.73% <ø> (ø)`
...tter/inject/requestscope/FinagleRequestScope.scala	`92.3% <ø> (ø)`
...r/finatra/thrift/exceptions/ExceptionManager.scala	`100% <ø> (ø)`
...http/internal/marshalling/MessageBodyManager.scala	`100% <100%> (ø)`
...e/src/main/scala/com/twitter/inject/Injector.scala	`100% <100%> (ø)`
...ra/http/exceptions/ExceptionMapperCollection.scala	`80% <80%> (ø)`

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 8200671...17b0b43. Read the comment docs.

cacoco commented 5 years ago

@xerial thanks for the PR, we'll take a look to see if we can merge it internally. Thanks again!

xerial commented 5 years ago

@cacoco Thanks. A major concern is the requirement of TypeTag for the inject method. If other components (e.g., TwitterServer) are using inject-core, we may need to update them as well.

cacoco commented 5 years ago

@xerial yes, it is a pretty major concern and I'm surprised by the switch to TypeTag since Manifest was un-deprecated in Scala 2.11. I think this is likely to break a lot of things internally though I will have to see.

In the interim, can you add exclusions on your Finatra dependency to exclude Guice libraries? We don't mark Guice as a "provided" dependency for obvious reasons but I wonder if you can get away with excluding the transitive dependencies from Finatra.

xerial commented 5 years ago

@cacoco Yes. We've already made a workaround by force upgrading to Guice 4.2.2 and scala-guice 4.2.3 in our project that uses TwitterServer. This seems safe for us so far since we are not directly using Finatra.

Take your time for reviewing this PR. I understand you have a lot of dependencies :)

cacoco commented 5 years ago

@xerial as expected this fails pretty badly internally and we'll have to scope out some time to work on updating, sorry.

I just want to confirm, if you are not using Finatra directly, I'm not sure how you're getting a Guice dependency. It is the only library in our suite of frameworks that has a Guice dependency. If you don't mind sharing your build, I don't understand how you get a Finatra dependency from just TwitterServer (the dependency graph should go the other way, Finatra depends on TwitterServer but TwitterServer has no dependency on Finatra). Likewise, Finagle itself has no Guice dependency.

Thanks!

xerial commented 5 years ago

Here is the dependency tree generated by sbt-dependency graph plugin. If we are using finatra-http, it will show such dependency:

> whatDependsOn net.codingwell scala-guice_2.12 4.1.0
...
[info] net.codingwell:scala-guice_2.12:4.1.0 [S]
[info]   +-com.twitter:inject-core_2.12:19.1.0 [S]
[info]     +-com.twitter:inject-app_2.12:19.1.0 [S]
[info]     | +-com.twitter:finatra-utils_2.12:19.1.0 [S]
[info]     | | +-com.twitter:finatra-http_2.12:19.1.0 [S]
[info]     | |   +-org.wvlet.airframe:airframe-http-finagle_2.12:19.3.4 [S]
[info]     | |

xerial commented 5 years ago

It seems we are using finatra-http, where finagle-http is sufficient. So we are not rushing to merge this PR. You can work later on this issue when you need to switch to Guice 4.2.2 or later.

But as the dependency tree suggest, using finatra-http involves Guice 4.0 dependency through inject-core -> scala-guice.

cacoco commented 5 years ago

@xerial Yes, if you bring in finatra-http, you will get a Guice dependency. But, this is only due to a direct dependency on something from Finatra. If you only need Finagle, as you mentioned you should switch to finagle-http (which finatra-http depends).

I'm going to close this for now, thanks for the start!

twitter / finatra

Upgrade to Guice 4.2.2 #491

Codecov Report