finagle-memcached: -> 213 #827
Conversation
|
I get the same errors with this branch as I get on develop on Windows. I haven't tested on a different platform if these errors are real, I'm assuming they're not. |
|
Threw in finagle-benchmark as it's just a build change. |
|
I looked at the travis-ci errors and all failed build jobs seem to fail during download of I cannot see any particular pattern regarding jdk or scala version and the sbt version should always be |
|
Yeah, the sbt download seems flaky. This is not the first time tests fail on that. |
There was a problem hiding this comment.
I haven't gotten really deep into this but I feel we're going to hit a non-trivial performance regression when dropping the breakOut usages in preference of .toMap and friends. From my reading it looks like 2.13 supports this via Views but they're probably only going to work for 2.13.
One way forward is to just reimplement what breakOut does. Since these use cases all have similar needs, specifically map to this new collection type, could we achieve the same result with a few helper methods?
|
For the breakout ones, we can go through an Iterator, which should avoid intermediary collection construction. For the mapValues ones, we can use 2.12.11 is expected soon(tm) scala/scala-dev#664 -- but of course, end users need to have that running too. |
There was a problem hiding this comment.
This looks good to me except Bryce's comments. I did source integration tests and this change passed. I think once we have util, scrooge, finagle, finatra, and twitter-server all cross-building 2.13 we will drop 2.11 support, so I won't be too worry about 2.11 perf.
finagle-memcached/src/main/scala/com/twitter/finagle/memcached/Client.scala
Outdated
Show resolved
Hide resolved
| @@ -822,7 +823,7 @@ protected class ConnectedClient(protected val service: Service[Command, Response | |||
| def stats(args: Option[String]): Future[Seq[String]] = { | |||
| val statArgs: Seq[Buf] = args match { | |||
| case None => Seq(Buf.Empty) | |||
| case Some(args) => args.split(" ").map(nonEmptyStringToBuf)(breakOut) | |||
| case Some(args) => args.split(" ").map(nonEmptyStringToBuf).toSeq | |||
There was a problem hiding this comment.
Could this also benefit from a .iterator call?
| @@ -59,7 +58,7 @@ trait TwemcacheConnectedClient extends TwemcacheClient { self: ConnectedClient = | |||
| def getvResult(keys: Iterable[String]): Future[GetsResult] = { | |||
| try { | |||
| if (keys == null) throw new IllegalArgumentException("Invalid keys: keys cannot be null") | |||
| val bufs = keys.map { Buf.Utf8(_) }(breakOut) | |||
| val bufs = keys.map { Buf.Utf8(_) }.toSeq | |||
There was a problem hiding this comment.
Does this need a .iterator call?
finagle-memcached/src/main/scala/com/twitter/finagle/memcached/TwemcacheCacheClient.scala
Outdated
Show resolved
Hide resolved
finagle-memcached/src/main/scala/com/twitter/finagle/memcached/util/Bufs.scala
Outdated
Show resolved
Hide resolved
| @@ -44,7 +44,7 @@ object ParserUtils { | |||
| segmentStart = segmentEnd + 1 | |||
| } | |||
| } | |||
| split | |||
| split.toIndexedSeq | |||
There was a problem hiding this comment.
Is this going to spit out a Vector? If so, that would be pretty bad.
There was a problem hiding this comment.
Yes. I pushed an approach in a separate commit that just toSeq's this and changes the return type to Seq (which then propagates forward a bit). I don't see any immediate performance hazards for that.
finagle-memcached/src/main/scala/com/twitter/finagle/memcached/Client.scala
Outdated
Show resolved
Hide resolved
finagle-memcached/src/main/scala/com/twitter/finagle/memcached/Client.scala
Show resolved
Hide resolved
|
Merged! 7acd423 |
Problem
no 2.13 for finagle-memcached
Solution
upgrade. Replace javaClientBase.java with equivalent .scala so that the type aliases of collection compat scala.jdk.CollectionConverters._ can be picked up.
Result
a 2.13 build