finagle-thriftmux: Make the push-based ThriftMux client the default

Summary: Problem / Solution The push-based ThriftMux client has been running in production for a while now so it seems safe enough to make it the default ThriftMux client muxer. JIRA Issues: CSL-6223 Differential Revision: https://phabricator.twitter.biz/D158134
twitter · Apr 26, 2018 · cc33315 · cc33315
1 parent 7ca095f
commit cc33315
Show file tree

Hide file tree

Showing 5 changed files with 7 additions and 31 deletions.
diff --git a/CHANGES b/CHANGES
@@ -62,6 +62,9 @@ Runtime Behavior Changes
   * finagle-http2: Clients and servers no longer attempt a cleartext upgrade if the
     first request of the HTTP/1.1 session has a body. ``PHAB_ID=D153986``
 
+  * finagle-thriftmux: The push-based client muxer is now the default muxer implementation.
+    The push-based muxer has better performance and a simpler architecture. ``PHAB_ID=D158134``
+
 Bug Fixes
 ~~~~~~~~~
 

diff --git a/...riftmux/src/main/resources/com/twitter/toggles/configs/com.twitter.finagle.thriftmux.json b/...riftmux/src/main/resources/com/twitter/toggles/configs/com.twitter.finagle.thriftmux.json
@@ -1,10 +1,5 @@
 {
   "toggles": [
-    {
-      "id": "com.twitter.finagle.thriftmux.UsePushMuxClient",
-      "description": "Use the push-based Mux implementation for the underlying client muxer",
-      "fraction": 0.0
-    },
     {
       "id": "com.twitter.finagle.thriftmux.UsePushMuxServer",
       "description": "Use the push-based Mux implementation for the underlying server muxer",

diff --git a/finagle-thriftmux/src/main/scala/com/twitter/finagle/ThriftMux.scala b/finagle-thriftmux/src/main/scala/com/twitter/finagle/ThriftMux.scala
@@ -122,11 +122,6 @@ object ThriftMux
 
   object Client extends ThriftClient {
 
-    private[finagle] val UsePushMuxClientToggleName =
-      "com.twitter.finagle.thriftmux.UsePushMuxClient"
-    private[this] val usePushMuxToggle = Toggles(UsePushMuxClientToggleName)
-    private[this] def UsePushMuxClient: Boolean = usePushMuxToggle(ServerInfo().id.hashCode)
-
     def apply(): Client =
       new Client()
         .withLabel("thrift")
@@ -141,9 +136,6 @@ object ThriftMux
       Mux.client
         .copy(stack = BaseClientStack)
         .configured(ProtocolLibrary("thriftmux"))
-
-    private def defaultMuxer: StackClient[mux.Request, mux.Response] =
-      if (UsePushMuxClient) pushMuxer else standardMuxer
   }
 
   /**
@@ -153,7 +145,7 @@ object ThriftMux
    * @see [[https://twitter.github.io/finagle/guide/Protocols.html#thrift Thrift]] documentation
    * @see [[https://twitter.github.io/finagle/guide/Protocols.html#mux Mux]] documentation
    */
-  case class Client(muxer: StackClient[mux.Request, mux.Response] = Client.defaultMuxer)
+  case class Client(muxer: StackClient[mux.Request, mux.Response] = Client.pushMuxer)
     extends StackBasedClient[ThriftClientRequest, Array[Byte]]
       with Stack.Parameterized[Client]
       with Stack.Transformable[Client]

diff --git a/finagle-thriftmux/src/test/scala/com/twitter/finagle/thriftmux/ThriftMuxTogglesTest.scala b/finagle-thriftmux/src/test/scala/com/twitter/finagle/thriftmux/ThriftMuxTogglesTest.scala
@@ -1,25 +1,11 @@
 package com.twitter.finagle.thriftmux
 
 import com.twitter.finagle.mux.exp.pushsession.MuxPush
-import com.twitter.finagle.{Mux, ThriftMux}
+import com.twitter.finagle.ThriftMux
 import com.twitter.finagle.toggle.flag
 import org.scalatest.FunSuite
 
 class ThriftMuxTogglesTest extends FunSuite {
-
-  // client tests
-  test("When toggle disabled use the transport-based Mux client as the default muxer") {
-    flag.overrides.let(ThriftMux.Client.UsePushMuxClientToggleName, 0.0) {
-      assert(ThriftMux.client.muxer.isInstanceOf[Mux.Client])
-    }
-  }
-
-  test("When toggle enabled use the push-based Mux client as the default muxer") {
-    flag.overrides.let(ThriftMux.Client.UsePushMuxClientToggleName, 1.0) {
-      assert(ThriftMux.client.muxer.isInstanceOf[MuxPush.Client])
-    }
-  }
-
   // server tests
   test("When toggle disabled use the transport-based Mux server as the default muxer") {
     flag.overrides.let(ThriftMux.Server.UsePushMuxServerToggleName, 0.0) {

diff --git a/finagle-thriftmux/src/test/scala/com/twitter/finagle/thriftmux/ssl/ThriftSmuxSslTest.scala b/finagle-thriftmux/src/test/scala/com/twitter/finagle/thriftmux/ssl/ThriftSmuxSslTest.scala
@@ -1,7 +1,7 @@
 package com.twitter.finagle.thriftmux.ssl
 
 import com.twitter.conversions.time._
-import com.twitter.finagle.{ChannelClosedException, Mux, SslVerificationFailedException}
+import com.twitter.finagle.{Mux, SslException, SslVerificationFailedException}
 import com.twitter.finagle.stats.InMemoryStatsReceiver
 import com.twitter.finagle.toggle.flag
 import com.twitter.finagle.thriftmux.ssl.ThriftSmuxSslTestComponents._
@@ -152,7 +152,7 @@ class ThriftSmuxSslTest extends FunSuite with Eventually {
       assertGaugeIsZero(serverStats, serverTlsConnections)
       assertGaugeIsZero(clientStats, clientTlsConnections)
 
-      intercept[ChannelClosedException] {
+      intercept[SslException] {
         Await.result(client.query("hello"), 2.seconds)
       }