search

twitter / finagle

A fault tolerant, protocol-agnostic RPC system
https://twitter.github.io/finagle
Apache License 2.0
8.78k stars 1.45k forks source link

Make MinSendBackupAfterMs of the BackupRequestFilter configurable #905

Closed peter-janssen closed 2 years ago

peter-janssen commented 3 years ago

The MinSendBackupAfterMs is now set to 1 ms. Unfortunately this causes backup requests to be send when there is low load. It would be useful if we could set the minimum a little bit higher for certain endpoints.

mosesn commented 3 years ago

@peter-janssen So sorry for the long delay in getting back to you!! I think that's pretty reasonable. How would you feel about making a PR?

tigerlily-he commented 3 years ago

Hi @peter-janssen, Friendly ping. Is configuring MinSendBackupAfterMs a relevant feature you still want?

rorye commented 2 years ago

Hi @tigerlily-he, I'm also interesting in configuring MinSendBackupAfterMs and am happy to implement this.

Where would be best to start, a new constructor parameter in BackupRequestFilter and updates to MethodBuilder?

tigerlily-he commented 2 years ago

Hi @rorye, Sorry for the delay.

Yes, start with adding MinSendBackupAfterMs to the stack param Configured in BackupRequestFilter like

private[client] case class Configured(
  maxExtraLoad: Tunable[Double],
  sendInterrupts: Boolean,
  minSendBackupAfterMs: Int = 1)
    extends Param

You will need to thread this minSendBackupAfterMs value through everywhere that has Param.Configured(maxExtraLoad, sendInterrupts) in order to pick up and use this stack param.

You will also need to update the def idempotent methods in MethodBuilder for finagle-core, finagle-http, and finagle-thriftmux to take in minSendBackupAfterMs. I recommend making a new idempotent for cleaner code change. The final API in MethodBullder might look something like for HTTP:

Http.client
  .withLabel("example_client")
  .methodBuilder("inet!localhost:8080")
  .idempotent(maxExtraLoad = 1.percent, minSendBackupAfterMs = 5)

Please add tests to BackupRequestFilterTest.scala. Let me know if want more guidance.

emilhotkowski commented 2 years ago

Hi @tigerlily-he I wouldn't want for your work to go to waste. Could I pick up this issue and help with it using your previous comment?

tigerlily-he commented 2 years ago

@emilhotkowski Of course! When you make the PR, you can request review from other participants on this issue so they can offer feedback.

emilhotkowski commented 2 years ago

Hi @tigerlily-he. Sorry it took so long but I finally have some PR for you to check out. Hopefully it is useful https://github.com/twitter/finagle/pull/923

tigerlily-he commented 2 years ago

Thanks for the PR @emilhotkowski! I'll leave a review over there.