felixbr commented 4 years ago

Problem

I would like to update our projects at work to Scala 2.13 but twitter-server is one of the missing dependencies and this made me sad. In order to not be sad anymore I want to do the update myself. 🙂

Solution

After adding the 2.13.1 version to the build I ran into a couple of compile-errors. I mostly replaced mutable.MutableList with mutable.ArrayBuffer and used .toSeq and .toList where needed to make the code compatible with 2.11, 2.12, and 2.13.

There were a few failing tests with 2 main causes:

The usage of .mapValues on a Map produces a lazy collection prior to 2.13 and a MapView in 2.13. The latter is serialized incorrectly by Jackson which can be fixed by a call to .toMap after .mapValues. I only found one occurrence in the whole project, so I opted for the local fix instead of a more general solution.
Several tests rely on the ordering of HashMaps in the test-code. This is already documented as a bad idea and because of the scala collection rework in 2.13 some tests started to fail only for 2.13. In these cases I wrote an assertion which first deserializes the json string using Jackson and compares what Jackson produces. This seems to work.

I tried to keep the changes minimal without really changing the semantic and style of the code (for example I would not use mutable collections so freely, but I kept it as is). I also only refactored the tests that failed instead of the whole test suite.

I hope this helps 🎉

Cheers ~ Felix

felixbr commented 4 years ago

Hm, fetching the source dependencies seems to fail for scala 2.13.1. I'm not too familiar with this dodo tool yet, so I appreciate any pointers what I could do to make this work. 🙂

jyanJing commented 4 years ago

Thank you @felixbr , looking!

cacoco commented 4 years ago

@felixbr thanks for the contribution. Dodo is explained here: http://github.com/twitter/dodo.

You will likely run into issues until all of Finagle is building with 2.13.x since it is a dependency and it is not completely moved yet (hence we have not moved TwitterServer yet). That is, in order to build TwitterServer for 2.13.x we need Finagle building with 2.13.x, the graph is very crudely, util -> finagle -> scrooge -> twitter-server (there's a circular dependency between parts of finagle and parts of scrooge but we'll ignore that for now). This is why the TravisCI build fails for 2.13.x, as Dodo is unable to build all of Finagle for 2.13.x and thus the necessary deps to build TwitterServer for 2.13.x are not in the ivy cache and building fails.

felixbr commented 4 years ago

Is there any chance to get this merged soon? All relevant finagle dependencies (finagle-core, finagle-http, finagle-toggle, and finagle-tunable) are already published for Scala 2.13.x, so technically this shouldn't be blocked anymore.

If you don't think this can be merged because finagle-integration and finagle-benchmark need to be migrated as well for dodo to work, I can see that but please tell me.

Our services at work are ready to upgrade to Scala 2.13.x and have only been blocked by twitter-server for a while now, so if this can't get merged we might consider publishing this branch internally until an official release is available.

Also, I can rebase this branch onto the current develop branch if that makes it easier for you to work with.

CLAassistant commented 4 years ago

All committers have signed the CLA.

felixbr commented 4 years ago

I rebased it onto the previous 20.1.0-SNAPSHOT base and confirmed that ./sbt +test is successful for all three Scala versions.

As I said, I can rebase it onto develop and resolve the merge-conflict, if you want me to, but I doubt it will pass CI anyway, as 20.2.0-SNAPSHOT seems not yet available.

cacoco commented 4 years ago

@felixbr thanks for the updates -- just to clarify, as mentioned in the CONTRIBUTING documentation, we don't publish snapshots. They are generated by the TravisCI build (via the Dodo script) for each project.

Please rebase onto the develop branch as that is necessary to be able to accept the PR, we can't accept PRs from master.

Thanks!

felixbr commented 4 years ago

Ok, I rebased onto develop and fixed the one merge-conflict.

Afterwards I verified that ./sbt +test still works for all three Scala versions.

codecov-io commented 4 years ago

Codecov Report

Merging #60 into develop will decrease coverage by 0.03%. The diff coverage is 66.66%.

@@             Coverage Diff             @@
##           develop      #60      +/-   ##
===========================================
- Coverage    66.42%   66.38%   -0.04%     
===========================================
  Files           73       73              
  Lines         1787     1791       +4     
  Branches       121      112       -9     
===========================================
+ Hits          1187     1189       +2     
- Misses         600      602       +2

Impacted Files	Coverage Δ
.../src/main/scala/com/twitter/server/Lifecycle.scala	`87.5% <ø> (ø)`	:arrow_up:
...twitter/server/handler/HistogramQueryHandler.scala	`47.1% <100%> (ø)`	:arrow_up:
...la/com/twitter/server/handler/TunableHandler.scala	`94.2% <100%> (ø)`	:arrow_up:
...erver/handler/logback/classic/LoggingHandler.scala	`81.39% <100%> (ø)`	:arrow_up:
...ala/com/twitter/server/handler/ToggleHandler.scala	`60.6% <50%> (-0.45%)`	:arrow_down:

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 910266c...7727df3. Read the comment docs.

caniszczyk commented 4 years ago

Codecov Report

Merging #60 into develop will decrease coverage by 0.03%. The diff coverage is n/a.

@@             Coverage Diff             @@
##           develop      #60      +/-   ##
===========================================
- Coverage    66.42%   66.38%   -0.04%     
===========================================
  Files           73       73              
  Lines         1787     1791       +4     
  Branches       121      112       -9     
===========================================
+ Hits          1187     1189       +2     
- Misses         600      602       +2

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 910266c...7727df3. Read the comment docs.

cacoco commented 4 years ago

@felixbr I know why TravisCI is failing to build correctly for Scala 2.13.1 here. Our util code is still cross building against 2.13.0 and TravisCI caches builds by exact Scala version. Hence when we try to build TwitterServer with Scala 2.13.1 it cannot find the previously built and cached items in the dependency graph as they're Scala 2.13.0. Let me update our other projects to Scala 2.13.1.

We should be able to remove the allowed failures workaround after updating all projects to Scala 2.13.1.

felixbr commented 4 years ago

Should I rebase onto develop again and resolve the merge conflicts?

edit: I'll just do it 🙂

felixbr commented 4 years ago

I think I messed up the PR, but I noticed you put the changes onto develop already, so it doesn't matter 😅🎉

cacoco commented 4 years ago

@felixbr yes this has landed here: 927c66f75c3d1493f83c74e8a333a8195aa7e13f. It looks like I somehow messed up the attribution. I will try to fix that -- this is your commit, my apologies!

felixbr commented 4 years ago

Don't worry too much about it 😅

I'm just happy that it can land in 20.3.0

felixbr commented 4 years ago

Thanks for taking the time to change it ♥️

cacoco commented 4 years ago

@felixbr of course! Sorry I flubbed this the first time.

felixbr commented 4 years ago

No worries, it might not have been your fault anyway, as GitHub seems to have changed their business logic for attribution around recently: https://twitter.com/greybaker/status/1235660378304131072

🙂

twitter / twitter-server

Scala: Crossbuild with 2.13.1 #60

Problem

Solution

Codecov Report

Codecov Report