Skip to content

Commit

Permalink
Finagle-Core: add MinSendBackupAfterMs to config in BackupRequestFilter
Browse files Browse the repository at this point in the history 
Problem

The `MinSendBackupAfterMs` is now set to 1 ms. Unfortunately
this causes backup requests to be send when there is low load.

Solution

Added `MinSendBackupAfterMs` to the stack param Configured in
BackupRequestFilter and propagated changes to MethodBuilder by
adding new versions of idempotent function.

Result

Developers will be able to set their own value of MinSendBackupAfterMs
if necessary.

Closes #905
fixes #923
Signed-off-by: Helen Woldesenbet <[email protected]>

JIRA Issues: CSL-11648

Differential Revision: https://phabricator.twitter.biz/D825503
  • Loading branch information
@emilhotkowski
emilhotkowski authored and jenkins committed Feb 23, 2022
1 parent c06e520 commit b0b8a6b
Show file tree
Hide file tree
Showing 8 changed files with 286 additions and 17 deletions.
5 changes: 5 additions & 0 deletions CHANGELOG.rst
View file
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,11 @@ New Features
* finagle-logging: Introduced SlowTracesFilter, which observes your requests and
logs the slowest ones that are also sampled for tracing. ``PHAB_ID=D813291``

* finagle-core: Introduced MinSendBackupAfterMs to the stack param Configured in
BackupRequestFilter and propagated changes to MethodBuilder by adding new versions of idempotent
function. When traffic load is low, this is useful to increase the delay when backup requests are
sent and prevent the client from sending unnecessary backup requests. ``PHAB_ID=D825503``

* finagle-core: Added a new annotation `clnt/has_dark_request` in tracing and Finagle
Local context. The new annotation can be used to indicate whether or not the request
has a span that is sent to dark service. ``PHAB_ID=D825317``
Expand Down
84 changes: 78 additions & 6 deletions finagle-core/src/main/scala/com/twitter/finagle/client/BackupRequestFilter.scala
View file
Original file line number Diff line number Diff line change
Expand Up @@ -86,7 +86,10 @@ object BackupRequestFilter {

object Param {

private[client] case class Configured(maxExtraLoad: Tunable[Double], sendInterrupts: Boolean)
private[client] case class Configured(
maxExtraLoad: Tunable[Double],
sendInterrupts: Boolean,
minSendBackupAfterMs: Int = MinSendBackupAfterMs)
extends Param
case object Disabled extends Param
implicit val param: Stack.Param[BackupRequestFilter.Param] = Stack.Param(Disabled)
Expand Down Expand Up @@ -127,6 +130,37 @@ object BackupRequestFilter {
* is returned and the result of the outstanding request is superseded. For
* protocols without a control plane, where the connection is cut on
* interrupts, this should be "false" to avoid connection churn.
*
* @param minSendBackupAfterMs Use a minimum non-zero delay to prevent sending unnecessary backup requests
* immediately for services where the latency at the percentile where a
* backup will be sent is ~0ms.
*/
def Configured(
maxExtraLoad: Double,
sendInterrupts: Boolean,
minSendBackupAfterMs: Int
): Param = {
require(
maxExtraLoad >= 0 && maxExtraLoad < 1.0,
s"maxExtraLoad must be between 0.0 and 1.0, was $maxExtraLoad"
)
require(
minSendBackupAfterMs >= 1,
s"minSendBackupAfterMs must be greater or equal to 1ms, was $minSendBackupAfterMs"
)
Param.Configured(Tunable.const(role.name, maxExtraLoad), sendInterrupts, minSendBackupAfterMs)
}

/**
* Configure [[BackupRequestFilter]].
*
* @param maxExtraLoad How much extra load, as a fraction, we are willing to send to the server.
* Must be between 0.0 and 1.0.
*
* @param sendInterrupts Whether or not to interrupt the original or backup request when a response
* is returned and the result of the outstanding request is superseded. For
* protocols without a control plane, where the connection is cut on
* interrupts, this should be "false" to avoid connection churn
*/
def Configured(maxExtraLoad: Double, sendInterrupts: Boolean): Param = {
require(
Expand All @@ -136,17 +170,41 @@ object BackupRequestFilter {
Param.Configured(Tunable.const(role.name, maxExtraLoad), sendInterrupts)
}

/**
* Configure [[BackupRequestFilter]].
*
* @param maxExtraLoad How much extra load, as a fraction, we are willing to send to the server.
* Must be between 0.0 and 1.0.
*
* @param sendInterrupts Whether or not to interrupt the original or backup request when a response
* is returned and the result of the outstanding request is superseded. For
* protocols without a control plane, where the connection is cut on
* interrupts, this should be "false" to avoid connection churn.
*
* @param minSendBackupAfterMs Use a minimum non-zero delay to prevent sending unnecessary backup
* requests immediately for services where the latency at the percentile
* where a backup will be sent is ~0ms.
*/
def Configured(
maxExtraLoad: Tunable[Double],
sendInterrupts: Boolean,
minSendBackupAfterMs: Int
): Param =
Param.Configured(maxExtraLoad, sendInterrupts, minSendBackupAfterMs)

def Configured(maxExtraLoad: Tunable[Double], sendInterrupts: Boolean): Param =
Param.Configured(maxExtraLoad, sendInterrupts)

private[this] def mkFilterFromParams[Req, Rep](
maxExtraLoad: Tunable[Double],
sendInterrupts: Boolean,
minSendBackupAfterMs: Int,
params: Stack.Params
): BackupRequestFilter[Req, Rep] =
new BackupRequestFilter[Req, Rep](
maxExtraLoad,
sendInterrupts,
minSendBackupAfterMs,
params[param.ResponseClassifier].responseClassifier,
params[Retries.Budget].retryBudget,
params[Histogram].lowestDiscernibleMsValue,
Expand Down Expand Up @@ -188,15 +246,17 @@ object BackupRequestFilter {
keyPrefixes: Seq[String]
): Service[Req, Rep] =
params[BackupRequestFilter.Param] match {
case BackupRequestFilter.Param.Configured(maxExtraLoad, sendInterrupts) =>
case BackupRequestFilter.Param
.Configured(maxExtraLoad, sendInterrupts, minSendBackupAfterMs) =>
// register BRF when registry prefixes are provided
if (keyPrefixes.nonEmpty) {
val value =
"maxExtraLoad: " + maxExtraLoad().toString + ", sendInterrupts: " + sendInterrupts
val prefixes = keyPrefixes ++ Seq(BackupRequestFilter.role.name, value)
ClientRegistry.export(params, prefixes: _*)
}
val brf = mkFilterFromParams[Req, Rep](maxExtraLoad, sendInterrupts, params)
val brf =
mkFilterFromParams[Req, Rep](maxExtraLoad, sendInterrupts, minSendBackupAfterMs, params)
new ServiceProxy[Req, Rep](brf.andThen(service)) {
override def close(deadline: Time): Future[Unit] =
service.close(deadline).before(brf.close(deadline))
Expand All @@ -220,10 +280,10 @@ object BackupRequestFilter {

def make(params: Params, next: ServiceFactory[Req, Rep]): ServiceFactory[Req, Rep] = {
params[BackupRequestFilter.Param] match {
case Param.Configured(maxExtraLoad, sendInterrupts) =>
case Param.Configured(maxExtraLoad, sendInterrupts, minSendBackupAfterMs) =>
new BackupRequestFactory[Req, Rep](
next,
mkFilterFromParams(maxExtraLoad, sendInterrupts, params)
mkFilterFromParams(maxExtraLoad, sendInterrupts, minSendBackupAfterMs, params)
)
case Param.Disabled =>
next
Expand Down Expand Up @@ -271,6 +331,13 @@ private[client] class BackupRequestFactory[Req, Rep](
* protocols without a control plane, where the connection is cut on
* interrupts, this should be "false" to avoid connection churn.
*
* @param minSendBackupAfterMs Use a minimum non-zero delay to prevent sending unnecessary backup requests
* immediately for services where the latency at the percentile where a backup will be sent is
* ~0ms. This is preferable to not sending any backups in the aforementioned case; by sending
* a backup after 1ms we can still reduce the higher latencies at greater latency percentiles.
* For example, if p99 latency is 0 and we are configured to send backups at the p99 latency,
* we can a reduce p999 latency of 10 ms to close to 1ms.
*
* @note If `sendInterrupts` is set to false, and for clients that mask interrupts (e.g. the
* Finagle Memcached client), both the original request and backup will be counted in stats,
* so tail latency improvements as a result of this filter will not be reflected in the
Expand All @@ -279,6 +346,7 @@ private[client] class BackupRequestFactory[Req, Rep](
private[finagle] class BackupRequestFilter[Req, Rep](
maxExtraLoadTunable: Tunable[Double],
sendInterrupts: Boolean,
minSendBackupAfterMs: Int,
responseClassifier: ResponseClassifier,
newRetryBudget: (Double, () => Long) => RetryBudget,
clientRetryBudget: RetryBudget,
Expand All @@ -293,6 +361,7 @@ private[finagle] class BackupRequestFilter[Req, Rep](
def this(
maxExtraLoadTunable: Tunable[Double],
sendInterrupts: Boolean,
minSendBackupAfterMs: Int,
responseClassifier: ResponseClassifier,
clientRetryBudget: RetryBudget,
statsReceiver: StatsReceiver,
Expand All @@ -301,6 +370,7 @@ private[finagle] class BackupRequestFilter[Req, Rep](
this(
maxExtraLoadTunable,
sendInterrupts,
minSendBackupAfterMs,
responseClassifier,
newRetryBudget = BackupRequestFilter.newRetryBudget,
clientRetryBudget = clientRetryBudget,
Expand All @@ -313,6 +383,7 @@ private[finagle] class BackupRequestFilter[Req, Rep](
def this(
maxExtraLoadTunable: Tunable[Double],
sendInterrupts: Boolean,
minSendBackupAfterMs: Int,
responseClassifier: ResponseClassifier,
clientRetryBudget: RetryBudget,
lowestDiscernibleMsValue: Int,
Expand All @@ -323,6 +394,7 @@ private[finagle] class BackupRequestFilter[Req, Rep](
this(
maxExtraLoadTunable,
sendInterrupts,
minSendBackupAfterMs,
responseClassifier,
newRetryBudget = BackupRequestFilter.newRetryBudget,
clientRetryBudget = clientRetryBudget,
Expand Down Expand Up @@ -375,7 +447,7 @@ private[finagle] class BackupRequestFilter[Req, Rep](
backupRequestRetryBudget = newRetryBudget(curMaxExtraLoad, nowMs)
}
sendBackupAfterMillis =
Math.max(MinSendBackupAfterMs, windowedPercentile.percentile(percentile))
Math.max(minSendBackupAfterMs, windowedPercentile.percentile(percentile))
sendAfterStat.add(sendBackupAfterMillis)
}
}
Expand Down
68 changes: 68 additions & 0 deletions finagle-core/src/main/scala/com/twitter/finagle/client/MethodBuilder.scala
View file
Original file line number Diff line number Diff line change
Expand Up @@ -229,6 +229,9 @@ final class MethodBuilder[Req, Rep] private[finagle] (
* is returned and the result of the outstanding request is superseded. For
* protocols without a control plane, where the connection is cut on
* interrupts, this should be "false" to avoid connection churn.
* @param minSendBackupAfterMs Use a minimum non-zero delay to prevent sending unnecessary backup requests
* immediately for services where the latency at the percentile where a
* backup will be sent is ~0ms.
* @param classifier [[ResponseClassifier]] (combined (via [[ResponseClassifier.orElse]])
* with any existing classifier in the stack params), used for determining
* whether or not requests have succeeded and should be retried.
Expand All @@ -237,6 +240,37 @@ final class MethodBuilder[Req, Rep] private[finagle] (
*
* @note See `idempotent` below for a version that takes a [[Tunable[Double]]] for `maxExtraLoad`.
*/
def idempotent(
maxExtraLoad: Double,
sendInterrupts: Boolean,
minSendBackupAfterMs: Int,
classifier: service.ResponseClassifier
): MethodBuilder[Req, Rep] = {
val brfParam =
if (maxExtraLoad == 0) BackupRequestFilter.Disabled
else BackupRequestFilter.Configured(maxExtraLoad, sendInterrupts, minSendBackupAfterMs)
addBackupRequestFilterParamAndClassifier(brfParam, classifier)
}

/**
* Configure that requests are to be treated as idempotent. Because requests can be safely
* retried, [[BackupRequestFilter]] is configured with the params maxExtraLoad and
* sendInterrupts to decrease tail latency by sending an additional fraction of requests.
*
* @param maxExtraLoad How much extra load, as a fraction, we are willing to send to the server.
* Must be between 0.0 and 1.0.
*
* @param sendInterrupts Whether or not to interrupt the original or backup request when a response
* is returned and the result of the outstanding request is superseded. For
* protocols without a control plane, where the connection is cut on
* interrupts, this should be "false" to avoid connection churn.
*
* @param classifier [[ResponseClassifier]] (combined (via [[ResponseClassifier.orElse]])
* with any existing classifier in the stack params), used for determining
* whether or not requests have succeeded and should be retried.
* These determinations are also reflected in stats, and used by
* [[FailureAccrualFactory]].
*/
def idempotent(
maxExtraLoad: Double,
sendInterrupts: Boolean,
Expand Down Expand Up @@ -266,6 +300,40 @@ final class MethodBuilder[Req, Rep] private[finagle] (
* is returned and the result of the outstanding request is superseded. For
* protocols without a control plane, where the connection is cut on
* interrupts, this should be "false" to avoid connection churn.
* @param minSendBackupAfterMs Use a minimum non-zero delay to prevent sending unnecessary backup requests
* immediately for services where the latency at the percentile where a
* backup will be sent is ~0ms.
* @param classifier [[ResponseClassifier]] (combined (via [[ResponseClassifier.orElse]])
* with any existing classifier in the stack params), used for determining
* whether or not requests have succeeded and should be retried.
* These determinations are also reflected in stats, and used by
* [[FailureAccrualFactory]].
*/
def idempotent(
maxExtraLoad: Tunable[Double],
sendInterrupts: Boolean,
minSendBackupAfterMs: Int,
classifier: service.ResponseClassifier
): MethodBuilder[Req, Rep] = {
addBackupRequestFilterParamAndClassifier(
BackupRequestFilter.Configured(maxExtraLoad, sendInterrupts, minSendBackupAfterMs),
classifier
)
}

/**
* Configure that requests are to be treated as idempotent. Because requests can be safely
* retried, [[BackupRequestFilter]] is configured with the params maxExtraLoad and
* sendInterrupts to decrease tail latency by sending an additional fraction of requests.
*
* @param maxExtraLoad How much extra load, as a fraction, we are willing to send to the server.
* Must be between 0.0 and 1.0.
*
* @param sendInterrupts Whether or not to interrupt the original or backup request when a response
* is returned and the result of the outstanding request is superseded. For
* protocols without a control plane, where the connection is cut on
* interrupts, this should be "false" to avoid connection churn.
*
* @param classifier [[ResponseClassifier]] (combined (via [[ResponseClassifier.orElse]])
* with any existing classifier in the stack params), used for determining
* whether or not requests have succeeded and should be retried.
Expand Down
Loading

0 comments on commit b0b8a6b

Please sign in to comment.