finagle-core: Make Hybrid FailureAccrual the Default

Problem Hybrid failure accrual has been running in production for over a year and so should be made the default failure accrual policy. Solution Remove the toggle for controlling which policy (hybrid, consecutive failures) acts as the default, and make hybrid the default. JIRA Issues: CSL-7897 Differential Revision: https://phabricator.twitter.biz/D327394
twitter · Jun 13, 2019 · 6f85c56 · 6f85c56
1 parent b6d658e
commit 6f85c56
Show file tree

Hide file tree

Showing 4 changed files with 32 additions and 57 deletions.
diff --git a/CHANGELOG.rst b/CHANGELOG.rst
@@ -35,6 +35,12 @@ New Features
 Runtime Behavior Changes
 ~~~~~~~~~~~~~~~~~~~~~~~~
 
+* finagle-core: The default failure accrual policy has been changed from one
+  which uses only consecutive failures to a hybrid model which uses both
+  success rate over a window and consecutive failures. Previously this was
+  changeable via toggle. The toggle has been removed, and the hybrid version
+  has been made the default. ``PHAB_ID=D327394``
+
 * finagle-http: Rename `request_stream_duration_ms` to `stream/request/duration_ms` and
   `response_stream_duration_ms` to `stream/response/duration_ms`. The stats will be
   populated when `isChunked` is set to true in the request and response respectively.

diff --git a/finagle-core/src/main/resources/com/twitter/toggles/configs/com.twitter.finagle.core.json b/finagle-core/src/main/resources/com/twitter/toggles/configs/com.twitter.finagle.core.json
@@ -1,10 +1,5 @@
 {
   "toggles": [
-    {
-      "id": "com.twitter.finagle.core.UseHybridFailureAccrual",
-      "description": "Use hybrid FailureAccrual by default",
-      "fraction": 0.0
-    },
     {
       "id": "com.twitter.util.BypassScheduler",
       "description": "Bypass the scheduler for Future.map operations (we expect this toggle to be temporary).",

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
@@ -33,42 +33,29 @@ object FailureAccrualFactory {
   private[finagle] val jitteredBackoff: Stream[Duration] =
     Backoff.equalJittered(5.seconds, 300.seconds)
 
-  private[finagle] def defaultPolicy: Function0[FailureAccrualPolicy] = {
-    if (useHybridDefaultPolicy) {
-      new Function0[FailureAccrualPolicy] {
-        def apply(): FailureAccrualPolicy =
-          FailureAccrualPolicy
-            .successRateWithinDuration(
-              DefaultSuccessRateThreshold,
-              DefaultSuccessRateWindow,
-              jitteredBackoff,
-              DefaultMinimumRequestThreshold
-            )
-            .orElse(
-              FailureAccrualPolicy
-                .consecutiveFailures(DefaultConsecutiveFailures, jitteredBackoff)
-            )
-
-        override def toString: String =
-          "FailureAccrualPolicy" +
-            ".successRateWithinDuration(" +
-            s"successRate = $DefaultSuccessRateThreshold, window = $DefaultSuccessRateWindow, " +
-            s"markDeadFor = $jitteredBackoff)" +
-            ".orElse(FailureAccrualPolicy" +
-            s".consecutiveFailures(numFailures: $DefaultConsecutiveFailures, markDeadFor: $jitteredBackoff)"
-      }
-    } else {
-      new Function0[FailureAccrualPolicy] {
-        def apply(): FailureAccrualPolicy =
-          FailureAccrualPolicy
-            .consecutiveFailures(DefaultConsecutiveFailures, jitteredBackoff)
-
-        override def toString: String =
-          s"FailureAccrualPolicy.consecutiveFailures(numFailures: " +
-            s"$DefaultConsecutiveFailures, markDeadFor: $jitteredBackoff)"
-      }
+  private[finagle] def defaultPolicy: Function0[FailureAccrualPolicy] =
+    new Function0[FailureAccrualPolicy] {
+      def apply(): FailureAccrualPolicy =
+        FailureAccrualPolicy
+          .successRateWithinDuration(
+            DefaultSuccessRateThreshold,
+            DefaultSuccessRateWindow,
+            jitteredBackoff,
+            DefaultMinimumRequestThreshold
+          )
+          .orElse(
+            FailureAccrualPolicy
+              .consecutiveFailures(DefaultConsecutiveFailures, jitteredBackoff)
+          )
+
+      override def toString: String =
+        "FailureAccrualPolicy" +
+          ".successRateWithinDuration(" +
+          s"successRate = $DefaultSuccessRateThreshold, window = $DefaultSuccessRateWindow, " +
+          s"markDeadFor = $jitteredBackoff)" +
+          ".orElse(FailureAccrualPolicy" +
+          s".consecutiveFailures(numFailures: $DefaultConsecutiveFailures, markDeadFor: $jitteredBackoff)"
     }
-  }
 
   /**
    * Add jitter in `markDeadFor` to reduce correlation.

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
@@ -4,21 +4,17 @@ import com.twitter.conversions.DurationOps._
 import com.twitter.finagle.stats.{InMemoryStatsReceiver, NullStatsReceiver}
 import com.twitter.finagle.service._
 import com.twitter.finagle._
-import com.twitter.finagle.toggle.flag
 import com.twitter.util._
 import java.util.concurrent.TimeUnit
-import org.junit.runner.RunWith
 import org.mockito.Mockito.{times, verify, when}
 import org.mockito.invocation.InvocationOnMock
 import org.mockito.stubbing.Answer
 import org.mockito.Matchers
 import org.mockito.Matchers._
 import org.scalatest.FunSuite
-import org.scalatest.junit.JUnitRunner
 import org.scalatest.mockito.MockitoSugar
 import scala.util.Random
 
-@RunWith(classOf[JUnitRunner])
 class FailureAccrualFactoryTest extends FunSuite with MockitoSugar {
 
   val markDeadFor = Backoff.equalJittered(5.seconds, 60.seconds)
@@ -63,23 +59,14 @@ class FailureAccrualFactoryTest extends FunSuite with MockitoSugar {
     verify(underlying)()
   }
 
-  test("default policy is consecutiveFailures") {
+  test("default policy is hybrid") {
+    val faf = FailureAccrualFactory.defaultPolicy.toString
     assert(
-      FailureAccrualFactory.defaultPolicy.toString
-        .contains("FailureAccrualPolicy.consecutiveFailures")
+      faf.contains("FailureAccrualPolicy.successRateWithinDuration") &&
+        faf.contains("FailureAccrualPolicy.consecutiveFailures")
     )
   }
 
-  test("default policy can be toggled to hybrid with window") {
-    flag.overrides.let("com.twitter.finagle.core.UseHybridFailureAccrual", 1.0) {
-      val faf = FailureAccrualFactory.defaultPolicy.toString
-      assert(
-        faf.contains("FailureAccrualPolicy.successRateWithinDuration") &&
-          faf.contains("FailureAccrualPolicy.consecutiveFailures")
-      )
-    }
-  }
-
   test("a failing service should become unavailable") {
     val h = new Helper(consecutiveFailures)
     import h._