finagle-core: remove toggle for apertureEagerConnections and enable b…

…y default

Problem/Solution

We have toggled up our apertureEagerConnections configuration to 100%
for over 6 months. Let's remove the toggle and enable it by default.

JIRA Issues: CSL-10757

Differential Revision: https://phabricator.twitter.biz/D625618

CHANGELOG.rst

@@ -22,6 +22,10 @@ New Features
 Breaking API Changes
 ~~~~~~~~~~~~~~~~~~~~
 
+* finagle-core: Changed flag `-com.twitter.finagle.loadbalancer.exp.apertureEagerConnections"
+  from having Boolean values true or false to `EagerConnectionsType` values `Enable`,
+  `Disable`, and `ForceWithDtab`. ``PHAB_ID=D613989``
+
 * finagle-mysql: The constructor of `c.t.f.mysql.transport.MysqlBufReader` now takes an underlying
   `c.t.io.ByteReader`. Prior uses of the constructor, which took a `c.t.io.Buf`, should migrate to
   using `c.t.f.mysql.transport.MysqlBufReader.apply` instead. ``PHAB_ID=D622705``
@@ -38,17 +42,21 @@ Runtime Behavior Changes
   backoffs. Services can convert a `Stream` to/from a `Backoff` with `Backoff.fromStream(Stream)`
   and `Backoff.toStream`. ``PHAB_ID=D592562``
 
+* finagle-core: remove the `com.twitter.finagle.loadbalancer.apertureEagerConnections` Toggle and
+  change the default behavior to enable eager connections for `c.t.f.loadbalancer.ApertureLeastLoaded`
+  and `c.t.f.loadbalancer.AperturePeakEwma` load balancers. The state of the
+  `com.twitter.finagle.loadbalancer.apertureEagerConnections` GlobalFlag now also defaults to enable
+  this feature (`Enable`. You can disable this feature for all clients via setting the
+  `com.twitter.finagle.loadbalancer.apertureEagerConnections` GlobalFlag to `Disable` for your process.
+  (i.e. `-com.twitter.finagle.loadbalancer.apertureEagerConnections=Disable`).
+  ``PHAB_ID=D625618``
+
 Deprecations
 ~~~~~~~~~~~~
 * finagle-core: `Backoff.fromJava` is marked as deprecated, since the new `Backoff` is java-friendly.
   For services using `Stream.iterator` on the old `Backoff`, please use the new API
   `Backoff.toJavaIterator` to acquire a java-friendly iterator. ``PHAB_ID=D592562``
 
-Breaking API Changes
-~~~~~~~~~~~~~~~~~~~~
-* finagle-core: Changed flag `-com.twitter.finagle.loadbalancer.exp.apertureEagerConnections"
-  from having Boolean values true or false to `EagerConnectionsType` values `Enable`,
-  `Disable`, and `ForceWithDtab`. ``PHAB_ID=D613989``
 
 21.2.0
 ------

finagle-core/src/main/resources/com/twitter/toggles/configs/com.twitter.finagle.core.json

@@ -1,10 +1,5 @@
 {
   "toggles": [
-    {
-      "id": "com.twitter.finagle.loadbalancer.exp.apertureEagerConnections",
-      "description": "Eagerly connect to the downstream endpoints in the Aperture load balancer",
-      "fraction": 0.0
-    },
     {
       "id": "com.twitter.finagle.OffloadEarly",
       "description": "Hand off work to the Offload pool right after we enter the Finagle stack",

finagle-core/src/main/scala/com/twitter/finagle/loadbalancer/Balancers.scala

@@ -259,7 +259,7 @@ object Balancers {
       val sr = params[param.Stats].statsReceiver
       val timer = params[param.Timer].timer
       val label = params[param.Label].label
-      val eagerConnections = params[EagerConnections].isEnabled
+      val eagerConnections = params[EagerConnections].enabled
 
       val balancer = new ApertureLeastLoaded(
         endpoints,
@@ -355,7 +355,7 @@ object Balancers {
       val sr = params[param.Stats].statsReceiver
       val timer = params[param.Timer].timer
       val label = params[param.Label].label
-      val eagerConnections = params[EagerConnections].isEnabled
+      val eagerConnections = params[EagerConnections].enabled
 
       val balancer = new AperturePeakEwma(
         endpoints,

finagle-core/src/main/scala/com/twitter/finagle/loadbalancer/aperture/ApertureLeastLoaded.scala

@@ -28,15 +28,14 @@ private[loadbalancer] final class ApertureLeastLoaded[Req, Rep](
   protected val timer: Timer,
   protected val emptyException: NoBrokersAvailableException,
   protected val useDeterministicOrdering: Option[Boolean],
-  withEagerConnections: () => Boolean)
+  protected val eagerConnections: Boolean)
     extends Aperture[Req, Rep]
     with LeastLoaded[Req, Rep]
     with LoadBand[Req, Rep]
     with Expiration[Req, Rep]
     with Updating[Req, Rep] {
   require(minAperture > 0, s"minAperture must be > 0, but was $minAperture")
   protected[this] val maxEffortExhausted: Counter = statsReceiver.counter("max_effort_exhausted")
-  protected def eagerConnections: Boolean = withEagerConnections()
 
   // We set the idle time as a function of the aperture's smooth window.
   // The aperture growth is dampened by this window so after X windows

finagle-core/src/main/scala/com/twitter/finagle/loadbalancer/aperture/AperturePeakEwma.scala

@@ -31,15 +31,14 @@ private[loadbalancer] final class AperturePeakEwma[Req, Rep](
   protected val timer: Timer,
   protected val emptyException: NoBrokersAvailableException,
   protected val useDeterministicOrdering: Option[Boolean],
-  withEagerConnections: () => Boolean)
+  protected val eagerConnections: Boolean)
     extends Aperture[Req, Rep]
     with PeakEwma[Req, Rep]
     with LoadBand[Req, Rep]
     with Expiration[Req, Rep]
     with Updating[Req, Rep] {
   require(minAperture > 0, s"minAperture must be > 0, but was $minAperture")
   protected[this] val maxEffortExhausted: Counter = statsReceiver.counter("max_effort_exhausted")
-  protected def eagerConnections = withEagerConnections()
 
   // We set the idle time as a function of the aperture's smooth window.
   // The aperture growth is dampened by this window so after X windows

finagle-core/src/main/scala/com/twitter/finagle/loadbalancer/aperture/Params.scala

@@ -2,8 +2,7 @@ package com.twitter.finagle.loadbalancer.aperture
 
 import com.twitter.app.Flaggable
 import com.twitter.finagle.loadbalancer.aperture.EagerConnectionsType.{EagerConnectionsType}
-import com.twitter.finagle.{CoreToggles, Stack}
-import com.twitter.finagle.server.ServerInfo
+import com.twitter.finagle.Stack
 import com.twitter.finagle.loadbalancer.exp.apertureEagerConnections
 
 /**
@@ -15,8 +14,7 @@ import com.twitter.finagle.loadbalancer.exp.apertureEagerConnections
  */
 private[twitter] case class EagerConnections private (
   mode: EagerConnectionsType) {
-  val isEnabled: () => Boolean = () => mode != EagerConnectionsType.Disable
-  def enabled: Boolean = isEnabled()
+  def enabled: Boolean = mode != EagerConnectionsType.Disable
   def withForceDtab: Boolean = mode == EagerConnectionsType.ForceWithDtab
 }
 
@@ -38,31 +36,17 @@ private[twitter] object EagerConnectionsType extends Enumeration {
 }
 
 private[twitter] object EagerConnections {
-  private val toggle = CoreToggles(apertureEagerConnections.name)
 
-  // The flag value takes precedence over the toggle
-  private val default: EagerConnections = {
-    val flagVal = apertureEagerConnections.get
-    flagVal match {
-      case Some(x: EagerConnectionsType) => EagerConnections(x)
-      case _ =>
-        // we lazily grab the server id until the toggle is evaluated to ensure
-        // server initialization has completed.
-        val serverHashCode = ServerInfo().id.hashCode
-        // protect against the toggle not being present
-        if (toggle.isDefined && toggle(serverHashCode)) EagerConnections(enabled = true)
-        else EagerConnections(enabled = false)
-    }
-  }
+  // the default Stack.Param value takes into account the flag value
+  private[this] val default: EagerConnections = EagerConnections(apertureEagerConnections())
 
-  implicit val param: Stack.Param[EagerConnections] = {
+  implicit val param: Stack.Param[EagerConnections] =
     Stack.Param(default)
-  }
 
   /**
    * Creates this param enabling eager connections
    */
-  def apply(): EagerConnections = this(true)
+  def apply(): EagerConnections = apply(true)
 
   /**
    * Explicitly configure EagerConnections

finagle-core/src/main/scala/com/twitter/finagle/loadbalancer/flags.scala

@@ -53,7 +53,7 @@ package exp {
 
   object apertureEagerConnections
       extends GlobalFlag[EagerConnectionsType.Value](
-        EagerConnectionsType.Disable,
+        EagerConnectionsType.Enable,
         "enable aperture eager connections\n" +
           "\tAccepts one of 3 values of EagerConnectionType: Enable, Disable, and ForceWithDtab.\n" +
           "\tWhen enabled, the aperture load balancer will eagerly establish connections with\n" +

finagle-core/src/test/scala/com/twitter/finagle/loadbalancer/LoadDistributionTest.scala

@@ -86,7 +86,10 @@ abstract class LoadDistributionTest(newBalancerFactory: Rng => LoadBalancerFacto
     clients.foreach(sendAndWait(150))
 
     // Optimal load is 750 / 10 = 75.
-    assert(servers.forall(s => s.load <= 150))
+    // With eager connections enabled, we expect 1 more request per client.
+    // With eager connections disabled, we expect <= 150, with it enabled we expect <= 155
+    // (as we have 5 clients)
+    assert(servers.forall(s => s.load <= 155))
   }
 
   test("servers deploy") {

finagle-core/src/test/scala/com/twitter/finagle/loadbalancer/aperture/ApertureTest.scala

@@ -84,7 +84,7 @@ class ApertureTest extends FunSuite with ApertureSuite {
         timer = new NullTimer,
         emptyException = new NoBrokersAvailableException,
         useDeterministicOrdering = None,
-        withEagerConnections = () => false
+        eagerConnections = false
       )
     }
   }
@@ -106,7 +106,7 @@ class ApertureTest extends FunSuite with ApertureSuite {
       timer = new NullTimer,
       emptyException = new NoBrokersAvailableException,
       useDeterministicOrdering = Some(true),
-      withEagerConnections = () => false
+      eagerConnections = false
     )
 
     assert(!stats.gauges.contains(Seq("loadband", "offered_load_ema")))
@@ -130,7 +130,7 @@ class ApertureTest extends FunSuite with ApertureSuite {
       timer = new NullTimer,
       emptyException = new NoBrokersAvailableException,
       useDeterministicOrdering = Some(false),
-      withEagerConnections = () => false
+      eagerConnections = false
     )
 
     assert(stats.gauges.contains(Seq("loadband", "offered_load_ema")))
@@ -155,7 +155,7 @@ class ApertureTest extends FunSuite with ApertureSuite {
       timer = new NullTimer,
       emptyException = new NoBrokersAvailableException,
       useDeterministicOrdering = Some(false),
-      withEagerConnections = () => false
+      eagerConnections = false
     )
 
     assert(stats.gauges.contains(Seq("loadband", "offered_load_ema")))
@@ -181,7 +181,7 @@ class ApertureTest extends FunSuite with ApertureSuite {
       timer = new NullTimer,
       emptyException = new NoBrokersAvailableException,
       useDeterministicOrdering = Some(true),
-      withEagerConnections = () => true
+      eagerConnections = true
     )
     assert(factories.forall(_.total == 1))
 
@@ -208,7 +208,7 @@ class ApertureTest extends FunSuite with ApertureSuite {
       timer = new NullTimer,
       emptyException = new NoBrokersAvailableException,
       useDeterministicOrdering = Some(true),
-      withEagerConnections = () => false
+      eagerConnections = false
     )
     assert(stats.counters(Seq("rebuilds")) == 1)
 
@@ -239,7 +239,7 @@ class ApertureTest extends FunSuite with ApertureSuite {
       timer = new NullTimer,
       emptyException = new NoBrokersAvailableException,
       useDeterministicOrdering = Some(true),
-      withEagerConnections = () => false
+      eagerConnections = false
     )
     assert(stats.counters(Seq("rebuilds")) == 1)
 

finagle-core/src/test/scala/com/twitter/finagle/loadbalancer/aperture/ParamsTest.scala

@@ -22,4 +22,46 @@ class ParamsTest extends FunSuite {
     apertureEagerConnections.reset()
   }
 
+  test("EagerConnections respects the flag value without explicit param") {
+    val eagerConnections = EagerConnections.param.getDefault
+    assert(eagerConnections.enabled == true)
+
+    // if the param is explicitly configured the flag is ignored, always
+    apertureEagerConnections.let(EagerConnectionsType.Disable) {
+      val eagerConnectionsEnabed = EagerConnections()
+      assert(eagerConnectionsEnabed.enabled == true)
+    }
+
+    apertureEagerConnections.let(EagerConnectionsType.Enable) {
+      val eagerConnectionsEnabled = EagerConnections()
+      assert(eagerConnectionsEnabled.enabled == true)
+    }
+
+    apertureEagerConnections.let(EagerConnectionsType.Disable) {
+      val eagerConnectionsExplicitTrue = EagerConnections(true)
+      assert(eagerConnectionsExplicitTrue.enabled == true)
+    }
+
+    apertureEagerConnections.let(EagerConnectionsType.Enable) {
+      val eagerConnectionsExplicitFalse = EagerConnections(false)
+      assert(eagerConnectionsExplicitFalse.enabled == false)
+    }
+
+    // because the EagerConnections.param is a val, we cannot use GlobalFlag scoping
+    // after EagerConnections class loading and value assignment has occurred, so
+    // the scope will always be ignored. given that users can configure the stack param,
+    // it's likely not worth changing the vals to defs. if that changes, these tests would
+    // then honor the flag value
+
+    apertureEagerConnections.let(EagerConnectionsType.Disable) {
+      val eagerConnectionsDisabled = EagerConnections.param.getDefault
+      assert(eagerConnectionsDisabled.enabled == true) // ignored
+    }
+
+    apertureEagerConnections.let(EagerConnectionsType.Enable) {
+      val eagerConnectionsEnabled = EagerConnections.param.getDefault
+      assert(eagerConnectionsEnabled.enabled == true) // flag is ignored, this is from initial val
+    }
+
+  }
 }