From 749289aa010144ab100aae9b44fbbed77a371fb7 Mon Sep 17 00:00:00 2001 From: Anton Ivanov Date: Wed, 13 Nov 2024 19:03:06 +0000 Subject: [PATCH] [finagle-core] deprecate Backoff.equalJittered # Problem `equalJittered` is a strange Backoff policy: - the first duration is not jittered; - it's actually exponential, but we already have `exponentialJittered` policy; - its' name - `equal` - does not tell much (to me); - scaladoc says it "keeps half of the exponential growth", it's not clear what's meant by "half": it multiplies the original `startDuration` by 2 each time, similar to `exponentialJittered` policy; - scaladoc says it has "jitter between 0 and that amount", which can be confusing, because the jitter is between the current and the next duration; - it's hard to explain where `equalJittered` should be used instead of `exponentialJittered` and vice versa, they are very similar; # Solution Remove `equalJittered`, fallback to `exponentialJittered`. JIRA Issues: STOR-8883 Differential Revision: https://phabricator.twitter.biz/D1182535 --- CHANGELOG.rst | 4 +- doc/src/sphinx/Clients.rst | 2 +- .../finagle/service/BackoffBenchmark.scala | 9 --- .../scala/com/twitter/finagle/Backoff.scala | 62 ++----------------- .../liveness/FailureAccrualFactory.scala | 12 ++-- .../com/twitter/finagle/BackoffTest.scala | 34 ---------- .../liveness/FailureAccrualFactoryTest.scala | 21 ++++--- .../liveness/FailureAccrualPolicyTest.scala | 23 ++++--- 8 files changed, 43 insertions(+), 124 deletions(-) diff --git a/CHANGELOG.rst b/CHANGELOG.rst index bac90dfa513..784202a51ef 100644 --- a/CHANGELOG.rst +++ b/CHANGELOG.rst @@ -17,10 +17,10 @@ Runtime Behavior Changes * finagle-netty4: `EventLoopGroupTracker` now collects the distribution of cpu utilization by each netty thread and all_sockets instead of active_sockets. ``PHAB_ID=D1177719`` * finagle-core: `Backoff.exponentialJittered` now jitter the first duration. ``PHAB_ID=D1182252`` -* finalge-core: `Backoff.exponentialJittered` now uses a new range for jitters: `[dur/2; dur + dur/2]`. +* finagle-core: `Backoff.exponentialJittered` now uses a new range for jitters: `[dur/2; dur + dur/2]`. Previously it was `[0, dur)`, which could result in `next.duration < duration` for arbitrary long invocation chains. ``PHAB_ID=D1182252`` - +* finagle-core: `Backoff.equalJittered` is now deprecated and falls back to `exponentialJittered`. ``PHAB_ID=D1182535`` New Features ~~~~~~~~~~ diff --git a/doc/src/sphinx/Clients.rst b/doc/src/sphinx/Clients.rst index 7900282bf4f..0c9b8113f6d 100644 --- a/doc/src/sphinx/Clients.rst +++ b/doc/src/sphinx/Clients.rst @@ -371,7 +371,7 @@ The following example [#example]_ constructs an instance of ``RetryPolicy`` usin import com.twitter.conversions.DurationOps._ val policy: RetryPolicy[Try[Response]] = - RetryPolicy.backoff(Backoff.equalJittered(10.milliseconds, 10.seconds)) { + RetryPolicy.backoff(Backoff.exponentialJittered(10.milliseconds, 10.seconds)) { case Return(rep) if rep.status == Status.InternalServerError => true } diff --git a/finagle-benchmark/src/main/scala/com/twitter/finagle/service/BackoffBenchmark.scala b/finagle-benchmark/src/main/scala/com/twitter/finagle/service/BackoffBenchmark.scala index 9eb5675f550..5daf3cfa51a 100644 --- a/finagle-benchmark/src/main/scala/com/twitter/finagle/service/BackoffBenchmark.scala +++ b/finagle-benchmark/src/main/scala/com/twitter/finagle/service/BackoffBenchmark.scala @@ -18,9 +18,6 @@ class BackoffBenchmark extends StdBenchAnnotations { @Benchmark def constant(state: Constant): Duration = state.next() - @Benchmark - def equalJittered(state: EqualJittered): Duration = state.next() - @Benchmark def exponentialJittered(state: ExponentialJittered): Duration = state.next() @@ -50,12 +47,6 @@ object BackoffBenchmark { Backoff.const(10.seconds).concat(Backoff.const(300.seconds)) ) - @State(Scope.Thread) - class EqualJittered - extends BackoffState( - Backoff.equalJittered(5.seconds, 300.seconds).concat(Backoff.const(300.seconds)) - ) - @State(Scope.Thread) class ExponentialJittered extends BackoffState( diff --git a/finagle-core/src/main/scala/com/twitter/finagle/Backoff.scala b/finagle-core/src/main/scala/com/twitter/finagle/Backoff.scala index 0d804a1502d..c4b6d1f8022 100644 --- a/finagle-core/src/main/scala/com/twitter/finagle/Backoff.scala +++ b/finagle-core/src/main/scala/com/twitter/finagle/Backoff.scala @@ -120,8 +120,7 @@ object Backoff { * @param maximum must be greater than 0 and greater than or equal to * `start`. * - * @see [[exponentialJittered]] and [[equalJittered]] for alternative - * jittered approaches. + * @see [[exponentialJittered]] for alternative jittered approaches. */ def decorrelatedJittered(start: Duration, maximum: Duration): Backoff = { @@ -135,27 +134,9 @@ object Backoff { else new DecorrelatedJittered(start, maximum, Rng.threadLocal) } - /** - * Create backoffs that keep half of the exponential growth, and jitter - * between 0 and that amount. - * - * @param start must be greater than 0 and less than or equal to `maximum`. - * @param maximum must be greater than 0 and greater than or equal to - * `start`. - * - * @see [[decorrelatedJittered]] and [[exponentialJittered]] for alternative - * jittered approaches. - */ + @deprecated("User `.exponentialJittered(Duration, Duration)` instead", "2024-11-13") def equalJittered(start: Duration, maximum: Duration): Backoff = { - - require(start > Duration.Zero) - require(maximum > Duration.Zero) - require(start <= maximum) - - // compare start and maximum here to avoid one - // iteration of creating a new `EqualJittered`. - if (start == maximum) new Const(start) - else new EqualJittered(start, start, maximum, 1, Rng.threadLocal) + exponentialJittered(start, maximum) } /** @@ -175,8 +156,7 @@ object Backoff { * @param maximum must be greater than 0 and greater than or equal to * `start`. * - * @see [[decorrelatedJittered]] and [[equalJittered]] for alternative - * jittered approaches. + * @see [[decorrelatedJittered]] for alternative jittered approaches. */ def exponentialJittered(start: Duration, maximum: Duration): Backoff = { @@ -281,36 +261,6 @@ object Backoff { def isExhausted: Boolean = false } - /** @see [[Backoff.equalJittered]] as the api to create this strategy. */ - // exposed for testing - private[finagle] final class EqualJittered( - startDuration: Duration, - nextDuration: Duration, - maximum: Duration, - attempt: Int, - rng: Rng) - extends Backoff { - - // Don't shift left more than 62 bits to avoid - // Long overflow of the multiplier `shift`. - private[this] final val MaxBitShift = 62 - - def duration: Duration = nextDuration - - def next: Backoff = { - val shift = 1L << MaxBitShift.min(attempt - 1) - // in case of Long overflow - val halfExp = if (startDuration >= maximum / shift) maximum else startDuration * shift - val randomBackoff = Duration.fromNanoseconds(rng.nextLong(halfExp.inNanoseconds)) - - // in case of Long overflow - if (halfExp == maximum || halfExp >= maximum - randomBackoff) new Const(maximum) - else new EqualJittered(startDuration, halfExp + randomBackoff, maximum, attempt + 1, rng) - } - - def isExhausted: Boolean = false - } - /** @see [[Backoff.exponentialJittered]] as the api to create this strategy. */ // exposed for testing private[finagle] final class ExponentialJittered( @@ -427,14 +377,12 @@ object Backoff { * - Create backoffs that grow linearly, can be created via `Backoff.linear`. * 1. DecorrelatedJittered * - Create backoffs that jitter randomly between a start value and 3 times of that value, can be created via `Backoff.decorrelatedJittered`. - * 1. EqualJittered - * - Create backoffs that jitter between 0 and half of the exponential growth. Can be created via `Backoff.equalJittered`. * 1. ExponentialJittered * - Create backoffs that jitter randomly between value/2 and value+value/2 that grows exponentially by 2. Can be created via `Backoff.exponentialJittered`. * * @note A new [[Backoff]] will be created only when `next` is called. * @note None of the [[Backoff]]s are memoized, for strategies that involve - * randomness (`DecorrelatedJittered`, `EqualJittered` and + * randomness (`DecorrelatedJittered` and * `ExponentialJittered`), there is no way to foresee the next backoff * value. * @note All [[Backoff]]s are infinite unless using [[take(Int)]] to create a diff --git a/finagle-core/src/main/scala/com/twitter/finagle/liveness/FailureAccrualFactory.scala b/finagle-core/src/main/scala/com/twitter/finagle/liveness/FailureAccrualFactory.scala index d25062bead8..94dacf1e836 100644 --- a/finagle-core/src/main/scala/com/twitter/finagle/liveness/FailureAccrualFactory.scala +++ b/finagle-core/src/main/scala/com/twitter/finagle/liveness/FailureAccrualFactory.scala @@ -1,10 +1,13 @@ package com.twitter.finagle.liveness import com.twitter.conversions.DurationOps._ -import com.twitter.finagle.Stack.{Params, Role} +import com.twitter.finagle.Stack.Params +import com.twitter.finagle.Stack.Role import com.twitter.finagle._ import com.twitter.finagle.client.Transporter -import com.twitter.finagle.service.{ReqRep, ResponseClass, ResponseClassifier} +import com.twitter.finagle.service.ReqRep +import com.twitter.finagle.service.ResponseClass +import com.twitter.finagle.service.ResponseClassifier import com.twitter.finagle.stats.StatsReceiver import com.twitter.logging.Logger import com.twitter.util._ @@ -20,12 +23,13 @@ object FailureAccrualFactory { private[this] val DefaultMinimumRequestThreshold = FailureAccrualPolicy.DefaultMinimumRequestThreshold - // Use equalJittered backoff in order to wait more time in between + // Use exponentialJittered backoff in order to wait more time in between // each revival attempt on successive failures; if an endpoint has failed // previous requests, it is likely to do so again. The recent // "failure history" should influence how long to mark the endpoint // dead for. - private[finagle] val jitteredBackoff: Backoff = Backoff.equalJittered(5.seconds, 300.seconds) + private[finagle] val jitteredBackoff: Backoff = + Backoff.exponentialJittered(5.seconds, 300.seconds) private[finagle] def defaultPolicy: Function0[FailureAccrualPolicy] = new Function0[FailureAccrualPolicy] { diff --git a/finagle-core/src/test/scala/com/twitter/finagle/BackoffTest.scala b/finagle-core/src/test/scala/com/twitter/finagle/BackoffTest.scala index 0c8b9c30d14..b9999e003ea 100644 --- a/finagle-core/src/test/scala/com/twitter/finagle/BackoffTest.scala +++ b/finagle-core/src/test/scala/com/twitter/finagle/BackoffTest.scala @@ -2,7 +2,6 @@ package com.twitter.finagle import com.twitter.conversions.DurationOps._ import com.twitter.finagle.Backoff.DecorrelatedJittered -import com.twitter.finagle.Backoff.EqualJittered import com.twitter.finagle.Backoff.ExponentialJittered import com.twitter.finagle.util.Rng import com.twitter.util.Duration @@ -104,39 +103,6 @@ class BackoffTest extends AnyFunSuite with ScalaCheckDrivenPropertyChecks { } } - test("equalJittered") { - val equalGen = for { - startMs <- Gen.choose(1L, 1000L) - maxMs <- Gen.choose(startMs, startMs * 2) - seed <- Gen.choose(Long.MinValue, Long.MaxValue) - } yield (startMs, maxMs, seed) - - forAll(equalGen) { - case (startMs: Long, maxMs: Long, seed: Long) => - val rng = Rng(seed) - val backoff: Backoff = - new EqualJittered(startMs.millis, startMs.millis, maxMs.millis, 1, Rng(seed)) - val result: ArrayBuffer[Duration] = new ArrayBuffer[Duration]() - var start = startMs.millis - for (attempt <- 1 to 7) { - result.append(start) - start = nextStart(startMs.millis, maxMs.millis, rng, attempt) - } - verifyBackoff(backoff, result.toSeq, exhausted = false) - } - - def nextStart(start: Duration, maximum: Duration, rng: Rng, attempt: Int): Duration = { - val shift = 1L << (attempt - 1) - // in case of Long overflow - val halfExp = if (start >= maximum / shift) maximum else start * shift - val randomBackoff = Duration.fromNanoseconds(rng.nextLong(halfExp.inNanoseconds)) - - // in case of Long overflow - if (halfExp == maximum || halfExp >= maximum - randomBackoff) maximum - else halfExp + randomBackoff - } - } - test("exponentialJittered") { val exponentialGen = for { startNs <- Gen.choose(1L, Long.MaxValue) diff --git a/finagle-core/src/test/scala/com/twitter/finagle/liveness/FailureAccrualFactoryTest.scala b/finagle-core/src/test/scala/com/twitter/finagle/liveness/FailureAccrualFactoryTest.scala index 49a04a1bef3..89352cb80ab 100644 --- a/finagle-core/src/test/scala/com/twitter/finagle/liveness/FailureAccrualFactoryTest.scala +++ b/finagle-core/src/test/scala/com/twitter/finagle/liveness/FailureAccrualFactoryTest.scala @@ -1,12 +1,12 @@ package com.twitter.finagle.liveness import com.twitter.conversions.DurationOps._ -import com.twitter.finagle.Backoff.EqualJittered import com.twitter.finagle.service._ import com.twitter.finagle.stats.InMemoryStatsReceiver import com.twitter.finagle.stats.NullStatsReceiver import com.twitter.finagle.util.Rng import com.twitter.finagle.Backoff +import com.twitter.finagle.Backoff.ExponentialJittered import com.twitter.finagle._ import com.twitter.util._ import java.util.concurrent.TimeUnit @@ -22,11 +22,10 @@ import scala.util.Random import org.scalatest.funsuite.AnyFunSuite class FailureAccrualFactoryTest extends AnyFunSuite with MockitoSugar { - // since `EqualJittered` generates values randomly, we pass the seed + // since `ExponentialJittered` generates values randomly, we pass the seed // here in order to validate the values returned in the tests. def markDeadFor(seed: Long): Backoff = - new EqualJittered(5.seconds, 5.seconds, 60.seconds, 1, Rng(seed)) - def markDeadForList(seed: Long) = markDeadFor(seed).take(6) + new ExponentialJittered(5.seconds.inNanoseconds, 60.seconds.inNanoseconds, Rng(seed)) def consecutiveFailures(seed: Long): FailureAccrualPolicy = FailureAccrualPolicy.consecutiveFailures(3, markDeadFor(seed)) @@ -284,7 +283,9 @@ class FailureAccrualFactoryTest extends AnyFunSuite with MockitoSugar { // Backoff to verify against from the backoff passed to create a FailureAccrual policy // Should make sure to use the same seed - var backoffs = new EqualJittered(5.seconds, 5.seconds, 60.seconds, 1, Rng(8888)).take(6) + var backoffs = + new ExponentialJittered(5.seconds.inNanoseconds, 60.seconds.inNanoseconds, Rng(8888)) + .take(6) while (!backoffs.isExhausted) { assert(statsReceiver.counters.get(List("removals")) == Some(1)) assert(!factory.isAvailable) @@ -323,9 +324,11 @@ class FailureAccrualFactoryTest extends AnyFunSuite with MockitoSugar { test("backoff should be 5 minutes when stream runs out") { // Backoff to pass to create a FailureAccrual policy - val markDeadForFA = new EqualJittered(5.seconds, 5.seconds, 60.seconds, 1, Rng(7777)).take(3) + val markDeadForFA = + new ExponentialJittered(5.seconds.inNanoseconds, 60.seconds.inNanoseconds, Rng(7777)).take(3) // Backoff to verify, should use the same seed as the policy passed to FA - var markDeadFor = new EqualJittered(5.seconds, 5.seconds, 60.seconds, 1, Rng(7777)).take(3) + var markDeadFor = + new ExponentialJittered(5.seconds.inNanoseconds, 60.seconds.inNanoseconds, Rng(7777)).take(3) val statsReceiver = new InMemoryStatsReceiver() val underlyingService = mock[Service[Int, Int]] @@ -433,7 +436,9 @@ class FailureAccrualFactoryTest extends AnyFunSuite with MockitoSugar { // Backoff to verify against from the backoff passed to create a FailureAccrual policy // Should make sure to use the same seed - var markDeadFor = new EqualJittered(5.seconds, 5.seconds, 60.seconds, 1, Rng(9999)).take(6) + var markDeadFor = + new ExponentialJittered(5.seconds.inNanoseconds, 60.seconds.inNanoseconds, Rng(9999)) + .take(6) for (_ <- 1 until 6) { // After another failure, the service should be unavailable intercept[Exception] { diff --git a/finagle-core/src/test/scala/com/twitter/finagle/liveness/FailureAccrualPolicyTest.scala b/finagle-core/src/test/scala/com/twitter/finagle/liveness/FailureAccrualPolicyTest.scala index f01e1990b0b..88a97f744f1 100644 --- a/finagle-core/src/test/scala/com/twitter/finagle/liveness/FailureAccrualPolicyTest.scala +++ b/finagle-core/src/test/scala/com/twitter/finagle/liveness/FailureAccrualPolicyTest.scala @@ -2,7 +2,7 @@ package com.twitter.finagle.liveness import com.twitter.conversions.DurationOps._ import com.twitter.finagle.Backoff -import com.twitter.finagle.Backoff.EqualJittered +import com.twitter.finagle.Backoff.ExponentialJittered import com.twitter.finagle.util.Rng import com.twitter.util._ import org.scalatestplus.mockito.MockitoSugar @@ -11,10 +11,10 @@ import org.scalatest.funsuite.AnyFunSuite class FailureAccrualPolicyTest extends AnyFunSuite with MockitoSugar { private[this] val constantBackoff = Backoff.const(5.seconds) - // since `EqualJittered` generates values randomly, we pass the seed + // since `ExponentialJittered` generates values randomly, we pass the seed // here in order to validate the values returned in the tests. private[this] def expBackoff(seed: Long) = - new EqualJittered(5.seconds, 5.seconds, 60.seconds, 1, Rng(seed)) + new ExponentialJittered(5.seconds.inNanoseconds, 60.seconds.inNanoseconds, Rng(seed)) private[this] def expBackoffList(seed: Long) = expBackoff(seed).take(6) test("Consecutive failures policy: fail on nth attempt") { @@ -219,6 +219,7 @@ class FailureAccrualPolicyTest extends AnyFunSuite with MockitoSugar { expBackoffList(333), 5, Stopwatch.timeMillis) + val backoff = expBackoffList(333) timeControl.advance(30.seconds) @@ -226,7 +227,7 @@ class FailureAccrualPolicyTest extends AnyFunSuite with MockitoSugar { assert(policy.markDeadOnFailure() == None) assert(policy.markDeadOnFailure() == None) assert(policy.markDeadOnFailure() == None) - assert(policy.markDeadOnFailure() == Some(5.seconds)) + assert(policy.markDeadOnFailure() == Some(backoff.duration)) } } @@ -245,36 +246,40 @@ class FailureAccrualPolicyTest extends AnyFunSuite with MockitoSugar { test("Hybrid policy: fail on nth attempt") { val policy = hybridPolicy + val backoff = expBackoff(333) // same as in hybridPolicy assert(policy.markDeadOnFailure() == None) assert(policy.markDeadOnFailure() == None) - assert(policy.markDeadOnFailure() == Some(5.seconds)) + assert(policy.markDeadOnFailure() == Some(backoff.duration)) } test("Hybrid policy: failures reset to zero on revived()") { val policy = hybridPolicy + val backoff = expBackoff(333) // same as in hybridPolicy assert(policy.markDeadOnFailure() == None) policy.revived() assert(policy.markDeadOnFailure() == None) assert(policy.markDeadOnFailure() == None) - assert(policy.markDeadOnFailure() == Some(5.seconds)) + assert(policy.markDeadOnFailure() == Some(backoff.duration)) } test("Hybrid policy: failures reset to zero on success") { val policy = hybridPolicy + val backoff = expBackoff(333) // same as in hybridPolicy assert(policy.markDeadOnFailure() == None) policy.recordSuccess() assert(policy.markDeadOnFailure() == None) assert(policy.markDeadOnFailure() == None) - assert(policy.markDeadOnFailure() == Some(5.seconds)) + assert(policy.markDeadOnFailure() == Some(backoff.duration)) } test("Hybrid policy: uses windowed success rate as well as consecutive failure") { Time.withCurrentTimeFrozen { timeControl => val policy = hybridPolicy + val backoff = expBackoff(333) for (i <- 0 until 15) { policy.recordSuccess() @@ -284,13 +289,13 @@ class FailureAccrualPolicyTest extends AnyFunSuite with MockitoSugar { timeControl.advance(1.second) } - assert(policy.markDeadOnFailure() == Some(5.seconds)) + assert(policy.markDeadOnFailure() == Some(backoff.duration)) policy.revived() assert(policy.markDeadOnFailure() == None) assert(policy.markDeadOnFailure() == None) - assert(policy.markDeadOnFailure() == Some(5.seconds)) + assert(policy.markDeadOnFailure() == Some(backoff.duration)) } }