vtgateproxy: sticky random balancer and remote zone backup connections #593
Conversation
Make it available to the picker layer along with the pool type. Also rework how the locality is computed to do it as part of building the target host rather than deferring the comparisons to the sorter.
Using the zone local attribute inhjected by discovery (if it exists), update sticky_random so that it biases to only use the local zone connections if there are any available, otherwise fall back to remote.
| // try to pick numConnections from the front of the list (local zone) and numBackupConnections | ||
| // from the tail (remote zone). if that's not possible, just take the whole set | ||
| if len(allTargets[poolType]) >= b.numConnections+b.numBackupConns { | ||
| remoteOffset := len(allTargets[poolType]) - b.numBackupConns |
There was a problem hiding this comment.
I wonder if it's cleaner to have the sorter return two separate slices, one for local, one for not. (Or to call shuffleSort twice on two slices).
If allTargets has more local connections than numConnections, but fewer non-local than numBackupConns, we'll put some local connections in the non-local bucket I think. And vice-versa.
There was a problem hiding this comment.
I spent a bit of time thinking about this when writing the PR and then again after your comment. While I agree that splitting into two lists would make some stuff clearer, I actually think the way it's written produces the better outcome in all the ways we'd actually want in practice.
So to elaborate a bit, it's clear that we're fine in either the "happy path" when there are enough conns of each flavor, and also in the very degenerate case when there are fewer conns total than numConnections + numBackup and you have to take all of what you got.
This means we're in a case where there more than enough conns total, but not enough of either local or remote type. However I think the better outcome in that case is to still establish numConnections + numBackupConns total connections, even if you end up with the "wrong" mix of local / remote because there aren't enough to satisfy the ask.
If we all agree, then I think the current code does exactly that.
If not, then we're saying we'd rather make sure to only connect to <= numConnections local and <= numBackupConns remote, then I agree with you sorting into two lists would be a clearer way to achieve that.
Description
Implement a couple improvements to the vtgateproxy to support:
--num_backup_connectionsoption to include of vtgates in remote zonesTesting
Verified in dev by forcing a webapp host to use a hand-built proxy running on localhost
Running the client and looking at /channelz/ shows that sticky_random only uses the local conns when they're available.
Also verified that it's now picking one backup conn by including that in the debug output:
