loadbalancer-experimental: allow configuring the pending request penalty #2991
Conversation
Motivation: The pending request penalty is a way to try to avoid connections with lost of pending requests which results in a more even request distribution. However, the penalty is currently fixed which doesn't allow us to choose how to prioritize between the fastest hosts vs the most fair request distribution. Modifications: Make the penalty factor configurable.
| @@ -155,7 +158,7 @@ public final int score() { | |||
| // Add penalty for pending requests to account for "unaccounted" load. | |||
| // Penalty is the observed latency if known, else an arbitrarily high value which makes entities for which | |||
| // no latency data has yet been received (eg: request sent but not received), un-selectable. | |||
| final int pendingPenalty = (int) min(MAX_VALUE, (long) cPending * currentEWMA); | |||
| final int pendingPenalty = (int) min(MAX_VALUE, (long) cPending * pendingRequestPenalty * currentEWMA); | |||
There was a problem hiding this comment.
This would arguably be better as a double since most useful values of this will center around the low single digits, most likely 0 and 1 being the most common. Otoh, maybe it's not worth letting people try to overthink it to find if 0.5 is better than 1. Feedback welcome.
There was a problem hiding this comment.
- What range is currently expected for resulting
pendingPenalty? - Why do we use
longfor config parameters but cast it tointinternally?
There was a problem hiding this comment.
- realistic values range between 0 and probably 100 where 100 is effectively infinite.
- I used a long for consistency with other parameters such as
errorPenaltyandcancelPenalty, which could arguably be turned to ints themselves.
There was a problem hiding this comment.
I'm happy to switch from long to int but would prefer to do the same with the cancellation and error penalties if we do. That should make Integer.MAX_VALUE safe to use as without worrying about overflows. However, I suspect we'd want to go through a deprecation cycle for the long method. WDYT?
There was a problem hiding this comment.
int is easier to operate with. If there is no need to have values larger than Integer.MAX_VALUE, +1 for changing to longs. if you are sure nobody used long methods outside our providers, simply change the type. if there is a risk, we need to go through deprecation
...adbalancer-experimental/src/main/java/io/servicetalk/loadbalancer/OutlierDetectorConfig.java
Outdated
Show resolved
Hide resolved
...adbalancer-experimental/src/main/java/io/servicetalk/loadbalancer/OutlierDetectorConfig.java
Outdated
Show resolved
Hide resolved
| @@ -155,7 +158,7 @@ public final int score() { | |||
| // Add penalty for pending requests to account for "unaccounted" load. | |||
| // Penalty is the observed latency if known, else an arbitrarily high value which makes entities for which | |||
| // no latency data has yet been received (eg: request sent but not received), un-selectable. | |||
| final int pendingPenalty = (int) min(MAX_VALUE, (long) cPending * currentEWMA); | |||
| final int pendingPenalty = (int) min(MAX_VALUE, (long) cPending * pendingRequestPenalty * currentEWMA); | |||
There was a problem hiding this comment.
- What range is currently expected for resulting
pendingPenalty? - Why do we use
longfor config parameters but cast it tointinternally?
...adbalancer-experimental/src/main/java/io/servicetalk/loadbalancer/OutlierDetectorConfig.java
Outdated
Show resolved
Hide resolved
Motivation:
The pending request penalty is a way to try to avoid connections with lost of pending requests which results in a more even request distribution. However, the penalty is currently fixed which doesn't allow us to choose how to prioritize between the fastest hosts vs the most fair request distribution.
Modifications:
Make the penalty factor configurable.