Skip to content

Commit

Permalink
finagle-http: Rename the toggle and start again from 0%
Browse files Browse the repository at this point in the history 
Summary: Problem

The ping memory leaks make it untenable to keep using the old toggle name.

Solution

Let's switch to a new toggle, and split up the client and server toggles.

JIRA Issues: CSL-5880

Differential Revision: https://phabricator.twitter.biz/D130988
  • Loading branch information
@mosesn
mosesn authored and jenkins committed Jan 26, 2018
1 parent a376dc9 commit 0d96039
Show file tree
Hide file tree
Showing 5 changed files with 37 additions and 18 deletions.
4 changes: 4 additions & 0 deletions CHANGES
View file
Expand Up @@ -36,6 +36,10 @@ Breaking API Changes
* finagle base-http: `c.t.f.h.Request.multipart` has been removed.
Use `c.t.f.h.exp.MultipartDecoder` instead. ``PHAB_ID=D129158``

* finagle-http: Split the toggle 'c.t.f.h.UseH2C' into a client-side toggle and a
server-side toggle, named 'c.t.f.h.UseH2CClients', and 'c.t.f.h.UseH2CServers',
respectively. ``PHAB_ID=D130988``

Runtime Behavior Changes
~~~~~~~~~~~~~~~~~~~~~~~~

Expand Down
9 changes: 7 additions & 2 deletions finagle-http/src/main/resources/com/twitter/toggles/configs/com.twitter.finagle.http.json
View file
Expand Up @@ -6,8 +6,13 @@
"fraction": 0.0
},
{
"id": "com.twitter.finagle.http.UseH2C",
"description": "Use h2c as the protocol implementation if the connection is in cleartext. If it fails to upgrade to http/2, it will downgrade to http/1.1.",
"id": "com.twitter.finagle.http.UseH2CClients",
"description": "Use h2c as the protocol implementation on the client-side if the connection is in cleartext. If it fails to upgrade to http/2, it will downgrade to http/1.1.",
"fraction": 0.0
},
{
"id": "com.twitter.finagle.http.UseH2CServers",
"description": "Use h2c as the protocol implementation on the server-side if the connection is in cleartext. If it fails to upgrade to http/2, it will downgrade to http/1.1.",
"fraction": 0.0
},
{
Expand Down
12 changes: 8 additions & 4 deletions finagle-http/src/main/scala/com/twitter/finagle/Http.scala
View file
Expand Up @@ -52,8 +52,12 @@ object Http extends Client[Request, Response] with HttpRichClient with Server[Re
private[this] val underlying: Toggle[Int] = Toggles("com.twitter.finagle.http.UseH2")
def apply(): Boolean = underlying(ServerInfo().id.hashCode)
}
private[this] object useH2C {
private[this] val underlying: Toggle[Int] = Toggles("com.twitter.finagle.http.UseH2C")
private[this] object useH2CClients {
private[this] val underlying: Toggle[Int] = Toggles("com.twitter.finagle.http.UseH2CClients")
def apply(): Boolean = underlying(ServerInfo().id.hashCode)
}
private[this] object useH2CServers {
private[this] val underlying: Toggle[Int] = Toggles("com.twitter.finagle.http.UseH2CServers")
def apply(): Boolean = underlying(ServerInfo().id.hashCode)
}

Expand Down Expand Up @@ -377,7 +381,7 @@ object Http extends Client[Request, Response] with HttpRichClient with Server[Re
}
override def newClient(dest: Name, label0: String): ServiceFactory[Request, Response] = {
val shouldHttp2 =
if (params[Transport.ClientSsl].sslClientConfiguration == None) useH2C()
if (params[Transport.ClientSsl].sslClientConfiguration == None) useH2CClients()
else useH2()
val explicitlyConfigured = params.contains[HttpImpl]
val client = if (!explicitlyConfigured && shouldHttp2) this.configuredParams(Http2)
Expand Down Expand Up @@ -573,7 +577,7 @@ object Http extends Client[Request, Response] with HttpRichClient with Server[Re
addr: SocketAddress,
factory: ServiceFactory[Request, Response]): ListeningServer = {
val shouldHttp2 =
if (params[Transport.ServerSsl].sslServerConfiguration == None) useH2C()
if (params[Transport.ServerSsl].sslServerConfiguration == None) useH2CServers()
else useH2()
val explicitlyConfigured = params.contains[HttpImpl]
val server = if (!explicitlyConfigured && shouldHttp2) this.configuredParams(Http2)
Expand Down
13 changes: 8 additions & 5 deletions finagle-http/src/test/scala/com/twitter/finagle/http/Http2AlpnTest.scala
View file
Expand Up @@ -70,18 +70,19 @@ class Http2AlpnTest extends AbstractEndToEndTest {
for {
clientUseHttp2 <- Seq(1D, 0D)
serverUseHttp2 <- Seq(1D, 0D)
toggleName <- Seq("com.twitter.finagle.http.UseH2", "com.twitter.finagle.http.UseH2C")
clientToggleName <- Seq("com.twitter.finagle.http.UseH2", "com.twitter.finagle.http.UseH2CClients")
serverToggleName <- Seq("com.twitter.finagle.http.UseH2", "com.twitter.finagle.http.UseH2CServers")
} {
val sr = new InMemoryStatsReceiver()
val server = overrides.let(Map(toggleName -> serverUseHttp2)) {
val server = overrides.let(Map(serverToggleName -> serverUseHttp2)) {
finagle.Http.server
.withStatsReceiver(sr)
.withLabel("server")
.configured(Transport.ServerSsl(Some(serverConfiguration())))
.serve("localhost:*", initService)
}
val addr = server.boundAddress.asInstanceOf[InetSocketAddress]
val client = overrides.let(Map(toggleName -> clientUseHttp2)) {
val client = overrides.let(Map(clientToggleName -> clientUseHttp2)) {
finagle.Http.client
.withStatsReceiver(sr)
.configured(Transport.ClientSsl(Some(clientConfiguration())))
Expand All @@ -92,7 +93,8 @@ class Http2AlpnTest extends AbstractEndToEndTest {
if (
clientUseHttp2 == 1.0 &&
serverUseHttp2 == 1.0 &&
toggleName == "com.twitter.finagle.http.UseH2"
clientToggleName == "com.twitter.finagle.http.UseH2" &&
serverToggleName == "com.twitter.finagle.http.UseH2"
) {
assert(sr.counters.get(Seq("client", "upgrade", "success")) == Some(1),
"Failed to upgrade when both parties were toggled on")
Expand All @@ -102,7 +104,8 @@ class Http2AlpnTest extends AbstractEndToEndTest {
val clientStatus = if (clientUseHttp2 == 1) "on" else "off"
val serverStatus = if (serverUseHttp2 == 1) "on" else "off"
val errorMsg = s"Upgraded when the client was $clientStatus, the server was " +
s"$serverStatus, the toggle was $toggleName"
s"$serverStatus, the client toggle was $clientToggleName, the server toggle was " +
s"$serverToggleName"
assert(!sr.counters.contains(Seq("client", "upgrade", "success")), errorMsg)
assert(!sr.counters.contains(Seq("server", "upgrade", "success")), errorMsg)
}
Expand Down
17 changes: 10 additions & 7 deletions finagle-http/src/test/scala/com/twitter/finagle/http/Http2EndToEndTest.scala
View file
Expand Up @@ -143,17 +143,18 @@ class Http2EndToEndTest extends AbstractEndToEndTest {
for {
clientUseHttp2 <- Seq(1D, 0D)
serverUseHttp2 <- Seq(1D, 0D)
toggleName <- Seq("com.twitter.finagle.http.UseH2", "com.twitter.finagle.http.UseH2C")
clientToggleName <- Seq("com.twitter.finagle.http.UseH2", "com.twitter.finagle.http.UseH2CClients")
serverToggleName <- Seq("com.twitter.finagle.http.UseH2", "com.twitter.finagle.http.UseH2CServers")
} {
val sr = new InMemoryStatsReceiver()
val server = overrides.let(Map(toggleName -> serverUseHttp2)) {
val server = overrides.let(Map(serverToggleName -> serverUseHttp2)) {
finagle.Http.server
.withStatsReceiver(sr)
.withLabel("server")
.serve("localhost:*", initService)
}
val addr = server.boundAddress.asInstanceOf[InetSocketAddress]
val client = overrides.let(Map(toggleName -> clientUseHttp2)) {
val client = overrides.let(Map(clientToggleName -> clientUseHttp2)) {
finagle.Http.client
.withStatsReceiver(sr)
.newService(s"${addr.getHostName}:${addr.getPort}", "client")
Expand All @@ -163,7 +164,8 @@ class Http2EndToEndTest extends AbstractEndToEndTest {
if (
clientUseHttp2 == 1.0 &&
serverUseHttp2 == 1.0 &&
toggleName == "com.twitter.finagle.http.UseH2C"
clientToggleName == "com.twitter.finagle.http.UseH2CClients" &&
serverToggleName == "com.twitter.finagle.http.UseH2CServers"
) {
assert(sr.counters.get(Seq("client", "upgrade", "success")) == Some(1),
"Failed to upgrade when both parties were toggled on")
Expand All @@ -173,7 +175,8 @@ class Http2EndToEndTest extends AbstractEndToEndTest {
val clientStatus = if (clientUseHttp2 == 1) "on" else "off"
val serverStatus = if (serverUseHttp2 == 1) "on" else "off"
val errorMsg = s"Upgraded when the client was $clientStatus, the server was " +
s"$serverStatus, the toggle was $toggleName"
s"$serverStatus, the client toggle was $clientToggleName, the server toggle was " +
s"$serverToggleName"
assert(!sr.counters.contains(Seq("client", "upgrade", "success")), errorMsg)
assert(!sr.counters.contains(Seq("server", "upgrade", "success")), errorMsg)
}
Expand All @@ -191,7 +194,7 @@ class Http2EndToEndTest extends AbstractEndToEndTest {
.withLabel("server")
.serve("localhost:*", initService)
val addr = server.boundAddress.asInstanceOf[InetSocketAddress]
val client = overrides.let(Map("com.twitter.finagle.http.UseH2C" -> clientUseHttp2)) {
val client = overrides.let(Map("com.twitter.finagle.http.UseH2CClients" -> clientUseHttp2)) {
val c = finagle.Http.client
.withStatsReceiver(sr)

Expand Down Expand Up @@ -219,7 +222,7 @@ class Http2EndToEndTest extends AbstractEndToEndTest {
serverUseHttp2 <- Seq(1D, 0D)
} {
val sr = new InMemoryStatsReceiver()
val server = overrides.let(Map("com.twitter.finagle.http.UseH2C" -> serverUseHttp2)) {
val server = overrides.let(Map("com.twitter.finagle.http.UseH2CServers" -> serverUseHttp2)) {
val s = finagle.Http.server
.withStatsReceiver(sr)
.withLabel("server")
Expand Down

0 comments on commit 0d96039

Please sign in to comment.