diff --git a/CHANGELOG.rst b/CHANGELOG.rst index f9a53e9e339..b10c1cca230 100644 --- 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 index f552d4b7fb3..ea73fbc7ec6 100644 --- 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 index 57e8fd95a4b..bb02445af39 100644 --- 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