Revert "finagle/finagle-core: Fix bug that causes backup requests to …

…be disabled when configured via stack param in MethodBuilder" This reverts commit 00060579cb027ab3ee05f8b861c4baed1a98d1ba. Differential Revision: https://phabricator.twitter.biz/D1190074
twitter · Dec 18, 2024 · 8b6572d · 8b6572d
1 parent 53d6894
commit 8b6572d
Show file tree

Hide file tree

Showing 3 changed files with 16 additions and 126 deletions.
diff --git a/CHANGELOG.rst b/CHANGELOG.rst
@@ -39,9 +39,6 @@ Bug Fixes
 * finagle-memcached: Fixed support for running memcached tests with external memcached. Added README with
   instructions under finagle/finagle-memcached. ``PHAB_ID=D1120240``
 * finagle-core: Fixed a bug in `ExponentialJitteredBackoff` where `rng` can be 0. ``PHAB_ID=D1178528``
-* finagle-core: When `c.t.f.client.BackupRequestFilter` is configured via stack params,
-  use this configuration when using MethodBuilder if `idempotent`/`nonIdempotent` have
-  not been set. ``PHAB_ID=D1189436``
 
 
 Breaking API Changes

diff --git a/finagle-core/src/main/scala/com/twitter/finagle/client/MethodBuilder.scala b/finagle-core/src/main/scala/com/twitter/finagle/client/MethodBuilder.scala
@@ -123,7 +123,7 @@ object MethodBuilder {
     retry: MethodBuilderRetry.Config,
     timeout: MethodBuilderTimeout.Config,
     filter: Filter.TypeAgnostic = Filter.typeAgnosticIdentity,
-    backup: Option[BackupRequestFilter.Param] = None)
+    backup: BackupRequestFilter.Param = BackupRequestFilter.Param.Disabled)
 
   /** Used by the `ClientRegistry` */
   private[client] val RegistryKey = "methods"
@@ -396,7 +396,7 @@ final class MethodBuilder[Req, Rep] private[finagle] (
       dest,
       stack,
       params,
-      config.copy(backup = Some(brfParam))
+      config.copy(backup = brfParam)
     )
 
     base.withRetry.forClassifier(idempotentedConfigClassifier)
@@ -436,7 +436,7 @@ final class MethodBuilder[Req, Rep] private[finagle] (
       dest,
       stack,
       params,
-      config.copy(backup = Some(BackupRequestFilter.Param.Disabled))
+      config.copy(backup = BackupRequestFilter.Param.Disabled)
     ).withRetry.forClassifier(nonidempotentedConfigClassifier)
   }
 
@@ -677,16 +677,9 @@ final class MethodBuilder[Req, Rep] private[finagle] (
 
     val underlying = methodPool.get
 
-    // If the user has not configured the backup requests via MethodBuilder (via `idempotent`,
-    // of `nonIdempotent`), use the configuration from the params (Disabled if none available).
-    val backupRequestParams = config.backup match {
-      case Some(backupRequestParam) =>
-        params +
-          backupRequestParam +
-          param.ResponseClassifier(config.retry.responseClassifier)
-      case None =>
-        params + param.ResponseClassifier(config.retry.responseClassifier)
-    }
+    val backupRequestParams = params +
+      config.backup +
+      param.ResponseClassifier(config.retry.responseClassifier)
 
     // register BackupRequestFilter under the same prefixes as other method entries
     val prefixes = Seq(registryEntry().addr) ++ registryKeyPrefix(name)

diff --git a/finagle-core/src/test/scala/com/twitter/finagle/client/MethodBuilderTest.scala b/finagle-core/src/test/scala/com/twitter/finagle/client/MethodBuilderTest.scala
@@ -658,19 +658,16 @@ class MethodBuilderTest
 
     val stackClient = TestStackClient(stack, params)
     val initialMethodBuilder = MethodBuilder.from("with backups", stackClient)
-    assert(initialMethodBuilder.config.backup == None)
-
     val methodBuilder =
-      initialMethodBuilder.withConfig(
-        initialMethodBuilder.config.copy(backup = Some(configuredBrfParam)))
+      initialMethodBuilder.withConfig(initialMethodBuilder.config.copy(backup = configuredBrfParam))
 
     // Ensure BRF is configured before calling `nonIdempotent`
-    assert(methodBuilder.config.backup == Some(configuredBrfParam))
+    assert(methodBuilder.config.backup == configuredBrfParam)
 
     val nonIdempotentClient = methodBuilder.nonIdempotent
 
     // Ensure BRF is disabled after calling `nonIdempotent`
-    assert(nonIdempotentClient.config.backup == Some(BackupRequestFilter.Disabled))
+    assert(nonIdempotentClient.config.backup == BackupRequestFilter.Disabled)
   }
 
   test("nonIdempotent client keeps existing ResponseClassifier in params ") {
@@ -799,9 +796,8 @@ class MethodBuilderTest
       .idempotent(1.percent, sendInterrupts = true, classifier)
 
     mb.config.backup match {
-      case Some(
-            BackupRequestFilter.Param
-              .Configured(maxExtraLoadTunable, sendInterrupts, minSendBackupAfterMs)) =>
+      case BackupRequestFilter.Param
+            .Configured(maxExtraLoadTunable, sendInterrupts, minSendBackupAfterMs) =>
         assert(
           maxExtraLoadTunable().get == 1.percent && sendInterrupts && minSendBackupAfterMs == 1)
       case _ => fail("BackupRequestFilter not configured")
@@ -866,9 +862,8 @@ class MethodBuilderTest
       .idempotent(tunable, sendInterrupts = true, ResponseClassifier.Default)
 
     assert(
-      mb.config.backup == Some(
-        BackupRequestFilter
-          .Configured(tunable, sendInterrupts = true))
+      mb.config.backup == BackupRequestFilter
+        .Configured(tunable, sendInterrupts = true)
     )
   }
 
@@ -885,13 +880,12 @@ class MethodBuilderTest
       .idempotent(tunable, sendInterrupts = true, ResponseClassifier.Default)
 
     assert(
-      mb.config.backup == Some(
-        BackupRequestFilter
-          .Configured(tunable, sendInterrupts = true))
+      mb.config.backup == BackupRequestFilter
+        .Configured(tunable, sendInterrupts = true)
     )
 
     val nonIdempotentMB = mb.nonIdempotent
-    assert(nonIdempotentMB.config.backup == Some(BackupRequestFilter.Disabled))
+    assert(nonIdempotentMB.config.backup == BackupRequestFilter.Disabled)
   }
 
   test("idempotent combines existing classifier with new one") {
@@ -1128,100 +1122,6 @@ class MethodBuilderTest
     assert(client.params[Retries.Budget].retryBudget eq retryBudget)
   }
 
-  test(
-    "BackupRequestFilter is configured with passed-in stack params when not configured in MethodBuilder") {
-    val stats = new InMemoryStatsReceiver()
-    val timer = new MockTimer()
-    val params =
-      Stack.Params.empty +
-        param.Stats(stats) +
-        param.Timer(timer) +
-        BackupRequestFilter.Configured(maxExtraLoad = 0.01, sendInterrupts = true)
-
-    val svc: Service[Int, Int] = Service.mk { i =>
-      Future.value(i)
-    }
-
-    val stack = Stack.leaf(Stack.Role("test"), ServiceFactory.const(svc))
-
-    val stackClient = TestStackClient(stack, Stack.Params.empty).withParams(params)
-    val mb = MethodBuilder.from("mb", stackClient)
-
-    Time.withCurrentTimeFrozen { tc =>
-      val client = mb.newService("a_client")
-      awaitResult(client(1))
-
-      tc.advance(10.seconds)
-      timer.tick()
-      assert(stats.stats.contains(Seq("mb", "a_client", "backups", "send_backup_after_ms")))
-    }
-  }
-
-  test(
-    "BackupRequestFilter is configured with MethodBuilder idempotent configuration when also configured via stack params") {
-    val stats = new InMemoryStatsReceiver()
-    val timer = new MockTimer()
-    val params =
-      Stack.Params.empty +
-        param.Stats(stats) +
-        param.Timer(timer) +
-        BackupRequestFilter.Disabled
-
-    val svc: Service[Int, Int] = Service.mk { i =>
-      Future.value(i)
-    }
-
-    val stack = Stack.leaf(Stack.Role("test"), ServiceFactory.const(svc))
-
-    val stackClient = TestStackClient(stack, Stack.Params.empty).withParams(params)
-    val classifier: ResponseClassifier = ResponseClassifier.named("foo") {
-      case ReqRep(_, Throw(_: IndividualRequestTimeoutException)) =>
-        ResponseClass.RetryableFailure
-    }
-
-    // MB config should take precedence
-    val mb = MethodBuilder.from("mb", stackClient).idempotent(0.05, true, classifier)
-
-    Time.withCurrentTimeFrozen { tc =>
-      val client = mb.newService("a_client")
-      awaitResult(client(1))
-
-      tc.advance(10.seconds)
-      timer.tick()
-      assert(stats.stats.contains(Seq("mb", "a_client", "backups", "send_backup_after_ms")))
-    }
-  }
-
-  test(
-    "BackupRequestFilter is configured with MethodBuilder nonIdempotent configuration when also configured via stack params") {
-    val stats = new InMemoryStatsReceiver()
-    val timer = new MockTimer()
-    val params =
-      Stack.Params.empty +
-        param.Stats(stats) +
-        param.Timer(timer) +
-        BackupRequestFilter.Configured(maxExtraLoad = 0.01, sendInterrupts = true)
-
-    val svc: Service[Int, Int] = Service.mk { i =>
-      Future.value(i)
-    }
-
-    val stack = Stack.leaf(Stack.Role("test"), ServiceFactory.const(svc))
-
-    val stackClient = TestStackClient(stack, Stack.Params.empty).withParams(params)
-    // MB config should take precedence
-    val mb = MethodBuilder.from("mb", stackClient).nonIdempotent
-
-    Time.withCurrentTimeFrozen { tc =>
-      val client = mb.newService("a_client")
-      awaitResult(client(1))
-
-      tc.advance(10.seconds)
-      timer.tick()
-      assert(!stats.stats.contains(Seq("mb", "a_client", "backups", "send_backup_after_ms")))
-    }
-  }
-
   test("shares RetryBudget between methods") {
     val params = StackClient.defaultParams