-
Notifications
You must be signed in to change notification settings - Fork 863
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Round robin load balancing #96
Conversation
@@ -79,9 +80,13 @@ public BackendHealthCheckOptions(bool enabled, TimeSpan interval, TimeSpan timeo | |||
public BackendLoadBalancingOptions(LoadBalancingMode mode) | |||
{ | |||
Mode = mode; | |||
// Increment returns the new value and we want the first return value to be 0. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd make the first value random - that way all requests don't all start out hitting the same server.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
? Every request increments the counter for an even distribution. It shouldn't matter where it starts, I only wanted 0 because it conceptually meets expectations (start the the top of the list and cycle through all entries).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In a distributed deployment of YARP, all YARP instances would start out choosing the same target server if initial choice is const. There is some merit to making it random, but i reckon the benefit is minimal to negligible. In a practical deployment, it is unlikely that many instances of YARP would be (re-)started at precisely the same time, and even if they were, there is a lot of entropy that would help minimize hotspots to some extent. Further, anyone doing round-robin probably doesn't care about hotspots anyway.
@@ -41,7 +41,8 @@ public Task Invoke(HttpContext context) | |||
var endpoints = endpointsFeature?.Endpoints | |||
?? throw new InvalidOperationException("The AvailableBackendEndpoints collection was not set."); | |||
|
|||
var loadBalancingOptions = backend.Config.Value?.LoadBalancingOptions ?? default; | |||
var loadBalancingOptions = backend.Config.Value?.LoadBalancingOptions |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this being read per request?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because it can be configured per route/backend, and can be changed if they are reloaded.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we create an issue to move RoundRobinState out of BackendConfig?
#72 While we debated more complex backend data storage mechanisms, I wanted to try something simple first. Built in algorithms can avail themselves of built in structures so I added the round robin state tracker to the existing BackendLoadBalancingOptions structure.