diff --git a/CHANGELOG.rst b/CHANGELOG.rst index 255866c43d..4b463ddbcc 100644 --- a/CHANGELOG.rst +++ b/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 ------ 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 index b53ca748c3..5258b7fec7 100755 --- 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.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", diff --git a/finagle-core/src/main/scala/com/twitter/finagle/loadbalancer/Balancers.scala b/finagle-core/src/main/scala/com/twitter/finagle/loadbalancer/Balancers.scala index b0881e95e8..2bd601194b 100644 --- a/finagle-core/src/main/scala/com/twitter/finagle/loadbalancer/Balancers.scala +++ b/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, diff --git a/finagle-core/src/main/scala/com/twitter/finagle/loadbalancer/aperture/ApertureLeastLoaded.scala b/finagle-core/src/main/scala/com/twitter/finagle/loadbalancer/aperture/ApertureLeastLoaded.scala index 85f059c75c..babf32e579 100644 --- a/finagle-core/src/main/scala/com/twitter/finagle/loadbalancer/aperture/ApertureLeastLoaded.scala +++ b/finagle-core/src/main/scala/com/twitter/finagle/loadbalancer/aperture/ApertureLeastLoaded.scala @@ -28,7 +28,7 @@ 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] @@ -36,7 +36,6 @@ private[loadbalancer] final class ApertureLeastLoaded[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 diff --git a/finagle-core/src/main/scala/com/twitter/finagle/loadbalancer/aperture/AperturePeakEwma.scala b/finagle-core/src/main/scala/com/twitter/finagle/loadbalancer/aperture/AperturePeakEwma.scala index f87b076dc3..ffe7015a32 100644 --- a/finagle-core/src/main/scala/com/twitter/finagle/loadbalancer/aperture/AperturePeakEwma.scala +++ b/finagle-core/src/main/scala/com/twitter/finagle/loadbalancer/aperture/AperturePeakEwma.scala @@ -31,7 +31,7 @@ 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] @@ -39,7 +39,6 @@ private[loadbalancer] final class AperturePeakEwma[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 diff --git a/finagle-core/src/main/scala/com/twitter/finagle/loadbalancer/aperture/Params.scala b/finagle-core/src/main/scala/com/twitter/finagle/loadbalancer/aperture/Params.scala index 8efaa5b3ff..eed2aaa476 100644 --- a/finagle-core/src/main/scala/com/twitter/finagle/loadbalancer/aperture/Params.scala +++ b/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 diff --git a/finagle-core/src/main/scala/com/twitter/finagle/loadbalancer/flags.scala b/finagle-core/src/main/scala/com/twitter/finagle/loadbalancer/flags.scala index d04cfe5dd0..65f13cf2de 100644 --- a/finagle-core/src/main/scala/com/twitter/finagle/loadbalancer/flags.scala +++ b/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" + diff --git a/finagle-core/src/test/scala/com/twitter/finagle/loadbalancer/LoadDistributionTest.scala b/finagle-core/src/test/scala/com/twitter/finagle/loadbalancer/LoadDistributionTest.scala index 314f990900..2cba5fc625 100644 --- a/finagle-core/src/test/scala/com/twitter/finagle/loadbalancer/LoadDistributionTest.scala +++ b/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") { diff --git a/finagle-core/src/test/scala/com/twitter/finagle/loadbalancer/aperture/ApertureTest.scala b/finagle-core/src/test/scala/com/twitter/finagle/loadbalancer/aperture/ApertureTest.scala index f68f239ec8..0006f1047c 100644 --- a/finagle-core/src/test/scala/com/twitter/finagle/loadbalancer/aperture/ApertureTest.scala +++ b/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) diff --git a/finagle-core/src/test/scala/com/twitter/finagle/loadbalancer/aperture/ParamsTest.scala b/finagle-core/src/test/scala/com/twitter/finagle/loadbalancer/aperture/ParamsTest.scala index ce273d28bf..981cdb5fe8 100644 --- a/finagle-core/src/test/scala/com/twitter/finagle/loadbalancer/aperture/ParamsTest.scala +++ b/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 + } + + } }