A76: Improvements to the Ring Hash LB Policy #412
Conversation
There was a problem hiding this comment.
Thanks for writing this!
Overall, the design looks good, but I think there is one significant issue related to where we compute the request hash, since I don't think it will work to do it in the picker. That will probably need a bit more discussion to resolve.
Please let me know if you have any questions. Thanks!
|
@markdroth just checking in on this, I've made the updates regarding handling the empty header. I'll start testing an implementation for this internally, but in the meantime would appreciate another review. Thanks! |
A76-ring-hash-improvements.md
Outdated
| // Determine request hash. | ||
| using_random_hash = false; | ||
| if (config.request_hash_header.empty()) { | ||
| request_hash = call_attributes.hash; |
There was a problem hiding this comment.
Can't this also be empty? In which case you'd use a random hash, too?
There was a problem hiding this comment.
Yes, I think the behavior is a bit confusing as it it, since if request_hash_header is empty, and the call attribute is not set because there is no xDS config selector to set it to a random value, then this would result in a fix hash that always routes to the same endpoint on the ring. I updated the logic and the text to pick a random hash in this case.
There was a problem hiding this comment.
I would prefer to preserve our existing behavior in the xDS case, which is to fail the pick if the hash isn't set (see C-core impl). If we're using xDS and the hash is not set, then something is very wrong, and I think it's better to fail RPCs with a specific error message to make the problem obvious than it is to just distribute traffic randomly.
| ```proto | ||
| message RingHashLoadBalancingConfig { | ||
| // (existing fields omitted) | ||
| string request_hash_header = 3; |
There was a problem hiding this comment.
Note when implementing this that the JSON form of this field will be requestHashHeader, so that's what the implementation should use.
| @@ -221,9 +230,7 @@ considered the following alternative solutions: | |||
|
|
|||
| ## Implementation | |||
|
|
|||
| Implemented in Go: | |||
| - Allow setting the request hash key: https://github.com/grpc/grpc-go/pull/7170 | |||
There was a problem hiding this comment.
I think it's useful to link implementation PRs here. I would suggest putting this back.
| // Set by the xDS config selector. | ||
| request_hash = call_attributes.hash; | ||
| } else { | ||
| using_random_hash = true; |
There was a problem hiding this comment.
As per my comment elsewhere, I think this case should fail the RPC, just as we do today.
| first_index = ring.FindIndexForHash(request_hash); | ||
| requested_connection = false; | ||
| if !using_random_hash { | ||
| // Return based on A62 unchanged. |
There was a problem hiding this comment.
I think you mean A61, not A62.
A76-ring-hash-improvements.md
Outdated
| // Determine request hash. | ||
| using_random_hash = false; | ||
| if (config.request_hash_header.empty()) { | ||
| request_hash = call_attributes.hash; |
There was a problem hiding this comment.
I would prefer to preserve our existing behavior in the xDS case, which is to fail the pick if the hash isn't set (see C-core impl). If we're using xDS and the hash is not set, then something is very wrong, and I think it's better to fail RPCs with a specific error message to make the problem obvious than it is to just distribute traffic randomly.
Based on discussion in grpc/grpc#33356.