twitter / storehaus

Storehaus is a library that makes it easy to work with asynchronous key value stores
Apache License 2.0
464 stars 86 forks source link

Upgrate to Finagle 6.43.0 and preparing the path for Scala 2.12 #344

Open csaltos opened 7 years ago

csaltos commented 7 years ago

The main goal of this PR is to update Finagle to the new version 6.43.0 for using new and improve Finagle features and performance and also one day migrate Storehaus to Scala 2.12.

The Scala 2.10 cross build is still kept valid using Finagle 6.35.0 (new Finagle versions beyond 6.35.0 are not compatible with Scala 2.10 any more which includes, of course Finagle 6.43.0)

Scala 2.12 is not added to the cross builds yet, since the Scalding dependency is not yet available for Scala 2.12 (all the other dependencies are already Scala 2.12 compatible updating their versions)

csaltos commented 7 years ago

The style of the code, name of the classes and packages or folders can be changed to keep the original convention the project uses, of course.

Let me know if a change, adjustment or fix this PR requires to be accepted (at Talenteca.com we would love to have a newer Storehaus with Finagle 6.43.0 available)

johnynek commented 7 years ago

looks like we need to update the .travis.yml to add 2.12 to the tests and use jdk8 for the 2.11 branch.

Thanks for taking the time to keep the 2.10 build green. I appreciate you doing that.

johnynek commented 7 years ago

how does this relate to #342? It seems like we might want to merge this first and leverage the work here in that one?

csaltos commented 7 years ago

The #342 eliminates the support for Scala 2.10 while this one keeps the Scala 2.10 and also upgrades Finagle to a newer version for Scala 2.11 (and even Scala 2.12 when we're ready to do it).

The #342 is already updating Storehaus to Scala 2.12 so we can merge those parts from that PR (once the Scalding part is resolved because that's still pending since Scalding is not Scala 2.12 compatible yet)

Thank you for taking a look at Travis, if I can be of any help on that part, let me know.

csaltos commented 7 years ago

Great, thank you @ben-adazza. I'll try to fix this PR following your advice.

codecov-io commented 7 years ago

Codecov Report

Merging #344 into develop will increase coverage by 0.21%. The diff coverage is 37.7%.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop     #344      +/-   ##
===========================================
+ Coverage    61.48%   61.69%   +0.21%     
===========================================
  Files          119      129      +10     
  Lines         1807     1885      +78     
  Branches       118      114       -4     
===========================================
+ Hits          1111     1163      +52     
- Misses         696      722      +26
Impacted Files Coverage Δ
...scala/com/twitter/storehaus/mysql/MySqlStore.scala 96.84% <ø> (ø) :arrow_up:
...a/com/twitter/storehaus/mysql/MySqlLongStore.scala 50% <ø> (ø) :arrow_up:
...cala/com/twitter/storehaus/mysql/ValueMapper.scala 54.76% <ø> (ø) :arrow_up:
...a/com/twitter/storehaus/redis/RedisHashStore.scala 0% <0%> (ø) :arrow_up:
...orehaus/memcache/compat/MemcacheCompatClient.scala 0% <0%> (ø)
...com/twitter/storehaus/memcache/MemcacheStore.scala 44% <0%> (+1.69%) :arrow_up:
...1/com/twitter/storehaus/mysql/compat/package.scala 0% <0%> (ø)
...11/com/twitter/storehaus/http/compat/package.scala 0% <0%> (ø)
...itter/storehaus/http/compat/HttpCompatClient.scala 0% <0%> (ø)
...orehaus/memcache/compat/MemcacheCompatClient.scala 0% <0%> (ø)
... and 21 more

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 b02d72c...5b52bcc. Read the comment docs.

csaltos commented 7 years ago

Ready @ben-adazza and @johnynek, this PR is now building successfully with travis CI and working for Scala 2.10 and Scala 2.11 for the new Finagle version (with this PR we get closer and closer to upgrade to Scala 2.12 soon)

pankajroark commented 7 years ago

Curios what this PR is waiting on. @johnynek @ben-adazza is this ok to merge? Asking because there is some overlap with the other PR around scala 2.12 upgrade.

CLAassistant commented 5 years ago

CLA assistant check
All committers have signed the CLA.