finagle-core: Rename c.t.f.context.Retries to c.t.f.context.Requeues

jcrossley · jenkins · commit 3c27308e590b · 2023-11-01T19:17:39.000Z
Problem `c.t.f.context.Retries` is not an accurate name because it actually stores the number of *requeues* a request has had. Solution Rename it to `c.t.f.context.Requeues`. Differential Revision: https://phabricator.twitter.biz/D1104878
diff --git a/CHANGELOG.rst b/CHANGELOG.rst
@@ -32,7 +32,14 @@ Bug Fixes
 
 * finagle-core: Failed recvAddress on Linux due to the remote peer resetting connection should now
   be properly seen as a `c.t.f.ChannelClosedException` instead of a
-  `c.t.f.UnknownChannelException`. ``PHAB_ID=`D1104650`
+  `c.t.f.UnknownChannelException`. ``PHAB_ID=`D1104650``
+
+Breaking API Changes
+~~~~~~~~~~~~~~~~~~~~
+
+* finagle-core: The `c.t.f.context.Retries` context has been renamed to `c.t.f.context.Requeues` to reflect what it
+  actually contains -- the number of requeues a request has had (on the client immediately upstream). Requeues are
+  retries on write exceptions (i.e. the original request was never sent to the server). ``PHAB_ID=`D1104878``
 
 * finagle: Deposit budget once in MethodBuilder ``PHAB_ID=D1107653``
 
diff --git a/doc/src/sphinx/Contexts.rst b/doc/src/sphinx/Contexts.rst
@@ -72,12 +72,13 @@ Current request deadline
 ``com.twitter.finagle.context.Deadline.current`` —
 A broadcast ``Context`` that represents when the request should be completed by.
 
-Current retry attempt
+Current requeue attempt
 ~~~~~~~~~~~~~~~~~~~~~
 
-``com.twitter.finagle.context.Retries.current`` —
-A broadcast ``Context`` that represents which retry attempt this request is.
-Will have ``attempt`` set to 0 if the request is not a retry.
+``com.twitter.finagle.context.Requeues.current`` —
+A broadcast ``Context`` that represents which requeue attempt this request is. 
+Requeues are retries on write exceptions (i.e. the original request was never sent to the server).
+Will have ``attempt`` set to 0 if the request is not a requeue.
 
 Current TLS session
 ~~~~~~~~~~~~~~~~~~~
diff --git a/finagle-base-http/src/main/scala/com/twitter/finagle/http/codec/context/HttpContext.scala b/finagle-base-http/src/main/scala/com/twitter/finagle/http/codec/context/HttpContext.scala
@@ -1,10 +1,14 @@
 package com.twitter.finagle.http.codec.context
 
 import com.twitter.finagle.context.Contexts
-import com.twitter.finagle.http.{HeaderMap, Message}
+import com.twitter.finagle.http.HeaderMap
+import com.twitter.finagle.http.Message
 import com.twitter.finagle.util.LoadService
-import com.twitter.logging.{Level, Logger}
-import com.twitter.util.{Return, Throw, Try}
+import com.twitter.logging.Level
+import com.twitter.logging.Logger
+import com.twitter.util.Return
+import com.twitter.util.Throw
+import com.twitter.util.Try
 import scala.collection.mutable
 import scala.util.control.NonFatal
 
@@ -47,7 +51,7 @@ object HttpContext {
   private[this] val knownContextTypes: Array[HttpContext] = {
     Array[HttpContext](
       HttpDeadline,
-      HttpRetries,
+      HttpRequeues,
       HttpBackupRequest
     )
   }
diff --git a/finagle-base-http/src/main/scala/com/twitter/finagle/http/codec/context/HttpRetriesContext.scala b/finagle-base-http/src/main/scala/com/twitter/finagle/http/codec/context/HttpRetriesContext.scala
@@ -1,23 +1,26 @@
 package com.twitter.finagle.http.codec.context
 
-import com.twitter.finagle.context.{Contexts, Retries}
-import com.twitter.util.{Return, Throw, Try}
+import com.twitter.finagle.context.Contexts
+import com.twitter.finagle.context.Requeues
+import com.twitter.util.Return
+import com.twitter.util.Throw
+import com.twitter.util.Try
 import scala.util.control.NonFatal
 
-private object HttpRetries extends HttpContext {
+private object HttpRequeues extends HttpContext {
 
-  type ContextKeyType = Retries
-  val key: Contexts.broadcast.Key[Retries] = Retries
+  type ContextKeyType = Requeues
+  val key: Contexts.broadcast.Key[Requeues] = Requeues
 
-  def toHeader(retries: Retries): String = {
-    retries.attempt.toString
+  def toHeader(requeues: Requeues): String = {
+    requeues.attempt.toString
   }
 
-  def fromHeader(header: String): Try[Retries] = {
+  def fromHeader(header: String): Try[Requeues] = {
     try {
-      Return(Retries(header.toInt))
+      Return(Requeues(header.toInt))
     } catch {
-      case NonFatal(e) => Throw(new NumberFormatException)
+      case NonFatal(_) => Throw(new NumberFormatException)
     }
   }
 }
diff --git a/finagle-base-http/src/test/scala/com/twitter/finagle/http/codec/context/HttpContextTest.scala b/finagle-base-http/src/test/scala/com/twitter/finagle/http/codec/context/HttpContextTest.scala
@@ -1,8 +1,14 @@
 package com.twitter.finagle.http.codec.context
 
 import com.twitter.conversions.DurationOps._
-import com.twitter.finagle.context.{BackupRequest, Contexts, Deadline, Retries}
-import com.twitter.finagle.http.{Message, Method, Request, Version}
+import com.twitter.finagle.context.BackupRequest
+import com.twitter.finagle.context.Contexts
+import com.twitter.finagle.context.Deadline
+import com.twitter.finagle.context.Requeues
+import com.twitter.finagle.http.Message
+import com.twitter.finagle.http.Method
+import com.twitter.finagle.http.Request
+import com.twitter.finagle.http.Version
 import com.twitter.io.Buf
 import com.twitter.util.Try
 import org.scalatest.funsuite.AnyFunSuite
@@ -89,48 +95,48 @@ class HttpContextTest extends AnyFunSuite {
     }
   }
 
-  test("written request retries matches read request retries") {
+  test("written request requeues matches read request requeues") {
     val m = newMsg()
-    val writtenRetries = Retries(5)
-    Contexts.broadcast.let(Retries, writtenRetries) {
+    val writtenRequeues = Requeues(5)
+    Contexts.broadcast.let(Requeues, writtenRequeues) {
       HttpContext.write(m)
 
-      // Clear Retries in the Context
-      Contexts.broadcast.letClear(Retries) {
+      // Clear Requeues in the Context
+      Contexts.broadcast.letClear(Requeues) {
 
-        // Ensure the Retries was cleared
-        assert(Retries.current == None)
+        // Ensure the Requeues was cleared
+        assert(Requeues.current == None)
 
         HttpContext.read(m) {
-          val readRetries = Retries.current.get
-          assert(writtenRetries == readRetries)
+          val readRequeues = Requeues.current.get
+          assert(writtenRequeues == readRequeues)
         }
       }
     }
   }
 
   test("headers are set/replaced, not added") {
     val m = newMsg()
-    Contexts.broadcast.let(Retries, Retries(5)) {
+    Contexts.broadcast.let(Requeues, Requeues(5)) {
       HttpContext.write(m)
     }
 
-    assert(m.headerMap.getAll(HttpRetries.headerKey).size == 1)
+    assert(m.headerMap.getAll(HttpRequeues.headerKey).size == 1)
     HttpContext.read(m) {
-      assert(Contexts.broadcast.get(HttpRetries.key) == Some(Retries(5)))
+      assert(Contexts.broadcast.get(HttpRequeues.key) == Some(Requeues(5)))
     }
 
     // and again...
-    Contexts.broadcast.let(Retries, Retries(4)) {
+    Contexts.broadcast.let(Requeues, Requeues(4)) {
       HttpContext.write(m)
     }
 
     // Still just 1...
-    assert(m.headerMap.getAll(HttpRetries.headerKey).size == 1)
+    assert(m.headerMap.getAll(HttpRequeues.headerKey).size == 1)
 
     // Should be the last entry written
     HttpContext.read(m) {
-      assert(Contexts.broadcast.get(HttpRetries.key) == Some(Retries(4)))
+      assert(Contexts.broadcast.get(HttpRequeues.key) == Some(Requeues(4)))
     }
   }
 
diff --git a/finagle-base-http/src/test/scala/com/twitter/finagle/http/filter/ContextFilterTest.scala b/finagle-base-http/src/test/scala/com/twitter/finagle/http/filter/ContextFilterTest.scala
@@ -2,38 +2,43 @@ package com.twitter.finagle.http.filter
 
 import com.twitter.conversions.DurationOps._
 import com.twitter.finagle.Service
-import com.twitter.finagle.context.{Contexts, Deadline, Retries}
+import com.twitter.finagle.context.Contexts
+import com.twitter.finagle.context.Deadline
+import com.twitter.finagle.context.Requeues
 import com.twitter.finagle.http.codec.context.HttpContext
-import com.twitter.finagle.http.{Request, Response, Status}
-import com.twitter.util.{Await, Future}
+import com.twitter.finagle.http.Request
+import com.twitter.finagle.http.Response
+import com.twitter.finagle.http.Status
+import com.twitter.util.Await
+import com.twitter.util.Future
 import org.scalatest.funsuite.AnyFunSuite
 
 class ContextFilterTest extends AnyFunSuite {
 
   test("parses Finagle-Ctx headers") {
     val writtenDeadline = Deadline.ofTimeout(5.seconds)
-    val writtenRetries = Retries(5)
+    val writtenRequeues = Requeues(5)
     val service =
       new ClientContextFilter[Request, Response] andThen
         new ServerContextFilter[Request, Response] andThen
         Service.mk[Request, Response] { req =>
           assert(Deadline.current.get == writtenDeadline)
-          assert(Retries.current.get == writtenRetries)
+          assert(Requeues.current.get == writtenRequeues)
           Future.value(Response())
         }
 
     Contexts.broadcast.let(Deadline, writtenDeadline) {
-      Contexts.broadcast.let(Retries, writtenRetries) {
+      Contexts.broadcast.let(Requeues, writtenRequeues) {
         val req = Request()
         HttpContext.write(req)
 
-        // Clear the deadline/retries values in the context
+        // Clear the deadline/requeues values in the context
         Contexts.broadcast.letClearAll {
           // ensure the deadline was cleared
           assert(Deadline.current == None)
 
-          // ensure the retries was cleared
-          assert(Retries.current == None)
+          // ensure the requeues was cleared
+          assert(Requeues.current == None)
 
           val rsp = Await.result(service(req))
           assert(rsp.status == Status.Ok)
diff --git a/finagle-core/src/main/scala/com/twitter/finagle/client/StackClient.scala b/finagle-core/src/main/scala/com/twitter/finagle/client/StackClient.scala
@@ -380,7 +380,7 @@ object StackClient {
     stk.push(PrepFactory.module)
     stk.push(FactoryToService.module)
     stk.push(Retries.moduleRequeueable)
-    stk.push(ClearContextValueFilter.module(context.Retries))
+    stk.push(ClearContextValueFilter.module(context.Requeues))
     stk.push(ExceptionSourceFilter.module)
 
     /*
diff --git a/finagle-core/src/main/scala/com/twitter/finagle/context/Requeues.scala b/finagle-core/src/main/scala/com/twitter/finagle/context/Requeues.scala
@@ -0,0 +1,46 @@
+package com.twitter.finagle.context
+
+import com.twitter.io.Buf
+import com.twitter.io.BufByteWriter
+import com.twitter.io.ByteReader
+import com.twitter.util.Return
+import com.twitter.util.Throw
+import com.twitter.util.Try
+
+/**
+ * Requeues contains the number of times a request has been requeued.
+ *
+ * @param attempt which retry attempt this is. Will be 0
+ *                if the request is not a requeue.
+ */
+case class Requeues(attempt: Int)
+
+/**
+ * Note: The context id is "c.t.f.Retries" and not "c.t.f.Requeues" because we have renamed the
+ * Retries context to Requeues (to reflect what it actually contains, which is the requeues), but we
+ * don't want to break existing users/deployments using this context by changing the key.
+ */
+object Requeues extends Contexts.broadcast.Key[Requeues]("com.twitter.finagle.Retries") {
+
+  def current: Option[Requeues] =
+    Contexts.broadcast.get(Requeues)
+
+  override def marshal(requeues: Requeues): Buf = {
+    val bw = BufByteWriter.fixed(4)
+    bw.writeIntBE(requeues.attempt)
+    bw.owned()
+  }
+
+  override def tryUnmarshal(buf: Buf): Try[Requeues] = {
+    if (buf.length != 4) {
+      Throw(
+        new IllegalArgumentException(
+          s"Could not extract Requeues from Buf. Length ${buf.length} but required 4"
+        )
+      )
+    } else {
+      val attempt: Int = ByteReader(buf).readIntBE()
+      Return(Requeues(attempt))
+    }
+  }
+}
diff --git a/finagle-core/src/main/scala/com/twitter/finagle/context/Retries.scala b/finagle-core/src/main/scala/com/twitter/finagle/context/Retries.scala
diff --git a/finagle-core/src/main/scala/com/twitter/finagle/service/RequeueFilter.scala b/finagle-core/src/main/scala/com/twitter/finagle/service/RequeueFilter.scala
@@ -2,8 +2,11 @@ package com.twitter.finagle.service
 
 import com.twitter.finagle.context.Contexts
 import com.twitter.finagle.stats.StatsReceiver
-import com.twitter.finagle.tracing.{Annotation, Trace, TraceId}
-import com.twitter.finagle.{context, _}
+import com.twitter.finagle.tracing.Annotation
+import com.twitter.finagle.tracing.Trace
+import com.twitter.finagle.tracing.TraceId
+import com.twitter.finagle.context
+import com.twitter.finagle._
 import com.twitter.util._
 
 /**
@@ -66,7 +69,7 @@ private[finagle] class RequeueFilter[Req, Rep](
     retriesRemaining: Int,
     backoffs: Backoff
   ): Future[Rep] = {
-    Contexts.broadcast.let(context.Retries, context.Retries(attempt)) {
+    Contexts.broadcast.let(context.Requeues, context.Requeues(attempt)) {
       val trace = Trace()
       val shouldTrace = attempt > 0 && trace.isActivelyTracing
       if (shouldTrace) {
diff --git a/finagle-core/src/test/scala/com/twitter/finagle/filter/ClearContextValueFilterTest.scala b/finagle-core/src/test/scala/com/twitter/finagle/filter/ClearContextValueFilterTest.scala
@@ -2,9 +2,12 @@ package com.twitter.finagle.filter
 
 import com.twitter.conversions.DurationOps._
 import com.twitter.finagle._
-import com.twitter.finagle.context.{Contexts, Retries}
+import com.twitter.finagle.context.Contexts
+import com.twitter.finagle.context.Requeues
 import com.twitter.finagle.stack.nilStack
-import com.twitter.util.{Promise, Await, Future}
+import com.twitter.util.Promise
+import com.twitter.util.Await
+import com.twitter.util.Future
 import org.scalatest.funsuite.AnyFunSuite
 
 class ClearContextValueFilterTest extends AnyFunSuite {
@@ -20,16 +23,16 @@ class ClearContextValueFilterTest extends AnyFunSuite {
         def make(next: ServiceFactory[Unit, Unit]) =
           new SimpleFilter[Unit, Unit] {
             def apply(req: Unit, service: Service[Unit, Unit]): Future[Unit] = {
-              Contexts.broadcast.let(Retries, Retries(5)) {
-                assert(Retries.current == Some(Retries(5)))
+              Contexts.broadcast.let(Requeues, Requeues(5)) {
+                assert(Requeues.current == Some(Requeues(5)))
                 setContextFilterCalled.setDone()
                 service(req)
               }
             }
           }.andThen(next)
       }
 
-    val clearContextFilter = ClearContextValueFilter.module[Unit, Unit](Retries)
+    val clearContextFilter = ClearContextValueFilter.module[Unit, Unit](Requeues)
 
     val verifyContextClearedFilter =
       new Stack.Module0[ServiceFactory[Unit, Unit]] {
@@ -38,7 +41,7 @@ class ClearContextValueFilterTest extends AnyFunSuite {
         def make(next: ServiceFactory[Unit, Unit]) =
           new SimpleFilter[Unit, Unit] {
             def apply(req: Unit, service: Service[Unit, Unit]): Future[Unit] = {
-              assert(Retries.current == None)
+              assert(Requeues.current == None)
               verifyContextClearedFilterCalled.setDone()
               service(req)
             }
diff --git a/finagle-core/src/test/scala/com/twitter/finagle/service/RequeueFilterTest.scala b/finagle-core/src/test/scala/com/twitter/finagle/service/RequeueFilterTest.scala
diff --git a/finagle-http/src/test/scala/com/twitter/finagle/http/AbstractEndToEndTest.scala b/finagle-http/src/test/scala/com/twitter/finagle/http/AbstractEndToEndTest.scala
diff --git a/finagle-integration/src/test/scala/com/twitter/finagle/integration/ContextPropagationTest.scala b/finagle-integration/src/test/scala/com/twitter/finagle/integration/ContextPropagationTest.scala
