From 37defbb62eda855d32fba7afcccde2a9bb7c73cf Mon Sep 17 00:00:00 2001 From: Antoine Tollenaere Date: Thu, 3 Apr 2025 10:16:04 +0200 Subject: [PATCH 1/3] [A76] Clarify behaviors discovered during implementation I discovered a few behaviors that I'm not sure are desired (although I don't think they are harmful either), and that may cause confusion for implementors, so definitely worth noting: 1. If multiple concurrent RPCs use the same picker, and the picker has no endpoint in connecting state, then both may trigger a connection attempt, resulting in multiple endpoints leaving Idle. This happens until the picker is updated to update `picker_has_endpoint_in_connecting_state`. One thing we could do to avoid this, at least in Go, is to atomically set `picker_has_endpoint_in_connecting_state` to true when we trigger a connection attempt. 2. If we queued the pick because there was no endpoint in `Ready` state and there was endpoints in `Idle` or connecting state, then when the endpoint transitions to Ready, we may end up triggering a connection attempt again on the next pick for a given RPC. 3. The behavior is not specified when multiple endpoints have the same hash key. Looking at Envoy's code, I think there is no deliberate choice of endpoint in that case, so we could leave it unspecified, but I think it's worth clarifying. --- A76-ring-hash-improvements.md | 20 +++++++++++++++++--- 1 file changed, 17 insertions(+), 3 deletions(-) diff --git a/A76-ring-hash-improvements.md b/A76-ring-hash-improvements.md index a341de27d..a4f3baac5 100644 --- a/A76-ring-hash-improvements.md +++ b/A76-ring-hash-improvements.md @@ -157,11 +157,19 @@ if !using_random_hash { return ring[first_index].picker->Pick(...); ``` -This behavior ensures that a single RPC does not cause more than one endpoint to +This behavior ensures that a single RPC does not cause more than two endpoint to exit `IDLE` state at a time, and that a request missing the header does not incur extra latency in the common case where there is already at least one endpoint in `READY` state. It converges to picking a random endpoint, since each -request may eventually cause a random endpoint to go from `IDLE` to `READY`. +request may cause one or two random endpoints to go from `IDLE` to `READY`: one +if it randomly picks an `IDLE` endpoint, and one if the pick was queued and an +endpoint transitioned from `CONNECTING` to `READY`, but no other endpoint is +currently `CONNECTING`. + +Note that because triggering a connection attempt and transitioning to +`CONNECTING` is not synchronous, it is possible for multiple endpoints to exit +`IDLE` state if no endpoint is `CONNECTING` and multiple RPCs that use a random +hash are made concurrently on the same picker. ### Explicitly setting the endpoint hash key @@ -178,6 +186,10 @@ endpoint attribute to the value of [LbEndpoint.Metadata][LbEndpoint.Metadata] `envoy.lb` `hash_key` field, as described in [Envoy's documentation for the ring hash load balancer][envoy-ringhash]. +If multiple endpoints have the same `hash_key`, then which endpoint is chosen is +unspecified, and the implementation is free to choose any of the endpoint with a +matching `hash_key`. + ### Temporary environment variable protection Explicitly setting the request hash key will be gated by the @@ -230,7 +242,9 @@ considered the following alternative solutions: ## Implementation -Implemented in C-core in https://github.com/grpc/grpc/pull/38312. +Implemented in: +- C-core in https://github.com/grpc/grpc/pull/38312. +- Go: https://github.com/grpc/grpc-go/pull/8159 [A42]: A42-xds-ring-hash-lb-policy.md [A61]: A61-IPv4-IPv6-dualstack-backends.md From 75008fdb207f062060362df36ee0f9d7d0726909 Mon Sep 17 00:00:00 2001 From: Antoine Tollenaere Date: Fri, 4 Apr 2025 09:36:56 +0200 Subject: [PATCH 2/3] comments from mark, eric --- A76-ring-hash-improvements.md | 27 +++++++++++++++------------ 1 file changed, 15 insertions(+), 12 deletions(-) diff --git a/A76-ring-hash-improvements.md b/A76-ring-hash-improvements.md index a4f3baac5..0f659913b 100644 --- a/A76-ring-hash-improvements.md +++ b/A76-ring-hash-improvements.md @@ -158,18 +158,21 @@ return ring[first_index].picker->Pick(...); ``` This behavior ensures that a single RPC does not cause more than two endpoint to -exit `IDLE` state at a time, and that a request missing the header does not -incur extra latency in the common case where there is already at least one -endpoint in `READY` state. It converges to picking a random endpoint, since each -request may cause one or two random endpoints to go from `IDLE` to `READY`: one -if it randomly picks an `IDLE` endpoint, and one if the pick was queued and an -endpoint transitioned from `CONNECTING` to `READY`, but no other endpoint is -currently `CONNECTING`. - -Note that because triggering a connection attempt and transitioning to -`CONNECTING` is not synchronous, it is possible for multiple endpoints to exit -`IDLE` state if no endpoint is `CONNECTING` and multiple RPCs that use a random -hash are made concurrently on the same picker. +exit `IDLE` state, and that a request missing the header does not incur extra +latency in the common case where there is already at least one endpoint in +`READY` state. It converges to picking a random endpoint if the header is not +set, since each request may cause one or two random endpoints to go from `IDLE` +to `READY`: +- a first connection attempt if it randomly picks an `IDLE` endpoint and no + endpoint is currently `CONNECTING` +- a second connection attempt if the pick was queued and an endpoint + transitioned from `CONNECTING` to `READY`, but no other endpoint is currently + `CONNECTING`. + +Because pickers generated with `picker_has_endpoint_in_connecting_state` set to +false may be used by multiple requests until an endpoint transitions to +`CONNECTING` and a the picker is updated, we can still start multiple +simultaneous connection attempts for different RPCs. ### Explicitly setting the endpoint hash key From 797f9353c9fb6021f90383ab130bd4c5d2ae9680 Mon Sep 17 00:00:00 2001 From: Antoine Tollenaere Date: Fri, 16 May 2025 09:49:43 +0200 Subject: [PATCH 3/3] review from Doug --- A76-ring-hash-improvements.md | 18 ++++++++---------- 1 file changed, 8 insertions(+), 10 deletions(-) diff --git a/A76-ring-hash-improvements.md b/A76-ring-hash-improvements.md index 0f659913b..719004bdc 100644 --- a/A76-ring-hash-improvements.md +++ b/A76-ring-hash-improvements.md @@ -157,17 +157,15 @@ if !using_random_hash { return ring[first_index].picker->Pick(...); ``` -This behavior ensures that a single RPC does not cause more than two endpoint to -exit `IDLE` state, and that a request missing the header does not incur extra +This behavior ensures that a single RPC does not cause more than two endpoints +to exit `IDLE` state, and that a request missing the header does not incur extra latency in the common case where there is already at least one endpoint in `READY` state. It converges to picking a random endpoint if the header is not set, since each request may cause one or two random endpoints to go from `IDLE` -to `READY`: -- a first connection attempt if it randomly picks an `IDLE` endpoint and no - endpoint is currently `CONNECTING` -- a second connection attempt if the pick was queued and an endpoint - transitioned from `CONNECTING` to `READY`, but no other endpoint is currently - `CONNECTING`. +to `READY`: - a first connection attempt if it randomly picks an `IDLE` endpoint +and no endpoint is currently `CONNECTING` - a second connection attempt if the +pick was queued and an endpoint transitioned from `CONNECTING` to `READY`, but +no other endpoint is currently `CONNECTING`. Because pickers generated with `picker_has_endpoint_in_connecting_state` set to false may be used by multiple requests until an endpoint transitions to @@ -190,8 +188,8 @@ endpoint attribute to the value of [LbEndpoint.Metadata][LbEndpoint.Metadata] hash load balancer][envoy-ringhash]. If multiple endpoints have the same `hash_key`, then which endpoint is chosen is -unspecified, and the implementation is free to choose any of the endpoint with a -matching `hash_key`. +unspecified, and the implementation is free to choose any of the endpoints with +a matching `hash_key`. ### Temporary environment variable protection