Build for 2.13.0 #771
Comments
|
There is at least a dependency on utils: twitter/util#248 Scalatest also needs to be updated, probably other dependencies as well |
|
the next step here is scrooge, I believe |
|
we can start work on some of the pieces, like finagle-core (basically anything that doesn't depend on scrooge within finagle), if people want to start taking stabs at subprojects! |
|
Now that everything in scrooge is working except for scrooge-generator-tests, we're unblocked on everything in finagle. |
|
What is the significance of the asterisks on items in the issue description? |
|
If I had to guess, I'd say those are not blocked by dependencies and open to port atm. |
|
Yep, @martijnhoekstra is right. I updated the original post to note that, sorry for the confusion |
|
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. |
|
finagle-netty4 is already complete on develop |
|
Whoops! I marked it as complete and updated some new ones to be marked as "ready to go". |
|
early warning: base-http depends on implementation details of the pre-2.13 hash sets that are no longer exposed, and possibly no longer present. That's going to be a pain. |
|
😿 where's the problem? we can start thinking about how we want to handle it |
|
I expect the biggest issue at cookieMap, then with HeaderMap.
CookieMap makes extensive use of HashMaps underlying structure of linked
hash entries to represent named cookies. That structure is no longer
exposed.
Trying to glue that back up is probably not a good idea. Implementing a
multi map from scratch, or using one from somewhere else (there may be one
in collection contrib) may be best bets.
…On Thu, Sep 12, 2019, 18:09 Moses Nakamura ***@***.***> wrote:
😿 where's the problem? we can start thinking about how we want to handle
it
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#771?email_source=notifications&email_token=AAGXOEOQZR3Q2TGPGU7TVOLQJJSS5A5CNFSM4HXYH7NKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD6SNJBY#issuecomment-530896007>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAGXOEL4UWJK5KIMWFFVM4DQJJSS5ANCNFSM4HXYH7NA>
.
|
|
thrift, mux and thriftmux at #794 |
|
following up on base-http - the biggest issue I'm running in to is Header. This is conceptually a mutable multimap case-insentitive string -> string. It accomplishes this by overriding the elementHash and elementEquality. This is no longer possible in 2.13. Hopefully someone sees an option beyond using a different (hand-rolled? copy-pasted? 3rd party dependency?) map implementation, or wrapping all keys and overriding equals and hashcode, because those options are pretty unappealing. EDIT: Actually, hand-rolled might actually be a good idea. A HashMap doesn't fit that well in the first place due to the sparsity of the data. I'll hand-roll a trie with case-insensitive prefix path, and run that through the benchmarks. We can take it from there. |
|
Sorry, I just saw this now. Let me talk to the team and see if I can get a consensus on what other testing we would have to do to be comfortable with that kind of change. |
|
@martijnhoekstra are you imagining that we would stop extending HashMap? |
|
@mosesn yes, my current plan is to re-implement
|
|
@mosesn there is a very WIP-y WIP in https://github.com/martijnhoekstra/finagle/tree/http_base_213 The mutable Headers trie implementation is https://github.com/martijnhoekstra/finagle/blob/http_base_213/finagle-base-http/src/main/scala/com/twitter/finagle/http/Headers.scala I'm currently working on getting 2.12 correct on this design so that I can bench against the baseline. |
|
The trie approach seems pretty simple so far. Another idea folks were floating internally was to just go with a List and call it a day. Did you try benchmarking that? |
|
just a List[Header], with linear search add/put/remove and toLowerCase'ing both sides? The original code did try very hard to be clever enough to be fast, but I can give it a try. I haven't benchmarked a thing yet. |
|
well since you're building the trie anyway, let's start with that and see how it goes, and see how much worse List[Header] is? |
|
The current trie inplementation I have is hilariously slow on iteration - 10x vs the baseline. I probably messed something up. |
|
Looking for guiding review on #797 |
|
I've addressed |
|
And took on |
Problem no 2.13 cross build Solution Add Scala 2.13 to the project settings. Result finagle-benchmark-thrift compiles on 2.13 Ref #771 Signed-off-by: Ruben Oanta <roanta@twitter.com> GitOrigin-RevId: 13c353672ef7acfd9d6d9a9cb0686297dada23ad
Problem no 2.13 cross build Solution Add Scala 2.13 to the project settings. Result finagle-benchmark-thrift compiles on 2.13 Ref #771 Signed-off-by: Ruben Oanta <roanta@twitter.com> Differential Revision: https://phabricator.twitter.biz/D386687
Problem no 2.13 cross build Solution Use scala.jdk.CollectionConverters from collection-compat Replace usages of Unit as a value with () EntryTest - don't observe the order of keys in a map Result finagle-serversets compiles on 2.13 and tests are passing. Ref #771 Signed-off-by: Ruben Oanta <roanta@twitter.com> GitOrigin-RevId: 92aaae349043300c5d6cc64b6f45710cced007ca
Problem no 2.13 cross build Solution Use scala.jdk.CollectionConverters from collection-compat Replace usages of Unit as a value with () EntryTest - don't observe the order of keys in a map Result finagle-serversets compiles on 2.13 and tests are passing. Ref #771 Signed-off-by: Ruben Oanta <roanta@twitter.com> Differential Revision: https://phabricator.twitter.biz/D386670
|
Any update on when this will be available? |
|
Hi @chrisbenincasa, the progress of this is up-to-date, we just merged in finagle-base-http. We are taking an OSS community collaboration approach for this work so we don't have an accurate estimation, sorry. |
|
Thanks for the update, Yufan. Is there anything I can do to help? |
|
@chrisbenincasa if you want to take one of the unaccounted for libraries listed above, that would be great. |
|
to add on what @ryanoneill said, the one with * (right now finagle-memcached*) means it should not be blocked by other dependencies. |
|
Gotcha, thanks. Was looking into memcached a bit today but it looks like it's blocked on 2.13 in bijection (handled here twitter/bijection#287 by @martijnhoekstra). Other than that, it seems other modules are still blocked? |
|
@martijnhoekstra I am going to try to tackle |
|
@chrisbenincasa Thanks for looking at it, good find, we will need to work on the bijection dep first. I took another look at the project list, I think the |
|
@martijnhoekstra @yufangong I am handling finagle-opencensus-tracing as well #822 |
|
How can we identify whether there will be performance regressions on a given upgrade like what happened in #797 ? Is there a benchmark that is run automatically and set to fail if performance falls below some threshold, or some other mechanism I can use to determine if one of my update PRs regresses? Curious because I'm about to do |
|
twitter-bijection 2.13 support was merged, but it's not part of the dodo build, so when a release comes around, maybe the last loose ends can be tied down. On a related note, have you tried building on dotty yet? |
|
It seems we are waiting on the version of bijection to be updated in the private version of this repo before we can move forward, right? Is that the last blocker here? |
|
No, bijection is through. There is a PR for memcached and benchmarks: #827 That blocks integration and example. I think that's the last of it. Then I think it would be nice to get all CI on 2.13 too. |
|
Gotcha, sorry, I fell a bit behind on what was happening here. Is there anything I can do to help? |
|
I have no idea how finagle-doc works, but it's not blocked by any subprojects anymore. Maybe that can be upgraded too. Cross-project, twitter/twitter-server doesn't seem blocked anymore either. I'm not entirely sure what's still left to do for other projects, maybe it would be good to make a matrix for what's left to do and what's blocked (by what) for all OSS projects. For example, Finatra is blocked by twitter-server, and there is no CI for scrooge on 2.13. Having an overview of all project statuses would definitely help to find the places that are unblocked for further progress, and what is still todo but blocked. |
Technically it isn't blocked anymore as all finagle modules it uses are available for 2.13. Because of the way PR for reference: twitter/twitter-server#60 |
|
Just to keep track of everything: This leaves only |
|
@felixbr thanks! it seems covered in finagle-memcached. Please go ahead with finagle-integration. I'm planning to do a March release soon and willing to wait for all subjects on 2.13. |
|
There also is the otherwise unmentioned finagle-doc which looks to depend on other tooling (I didn't try getting it to build or run for very long) |
|
@yufangong did this make it into the release? |
|
@martinffx this will be in April release which will happen in the next couple days. |
|
We believe the most recent release 20.4.1 should have everything cross-building with Scala 2.13, I'm going to close this issue. |
Scala 2.13.0 was released, so it would be great to have finagle build for it.
If it's marked with an asterisk, it doesn't have any dependencies and will be easy to target next. If you want to discover what the dependencies of a given project are, take a peek inside of the build.sbt file.
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.
The text was updated successfully, but these errors were encountered: