[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

Author: Anton Ivanov

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
 ~~~~~~~~~~

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
     }
 

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(

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

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] {

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)

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] {

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,14 +219,15 @@ class FailureAccrualPolicyTest extends AnyFunSuite with MockitoSugar {
           expBackoffList(333),
           5,
           Stopwatch.timeMillis)
+      val backoff = expBackoffList(333)
 
       timeControl.advance(30.seconds)
 
       assert(policy.markDeadOnFailure() == None)
       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))
     }
   }