ringhash: allow setting request hash key explicitly #7170
Conversation
Implement part 1 of A76: the ability to set the request hash key, to extract the hash from a header. This allows using ring hash without xDS.
|
Ready to review, will wait for A76 to be merged before merging this. |
|
Sure, will take a look. |
| @@ -66,5 +69,10 @@ func parseConfig(c json.RawMessage) (*LBConfig, error) { | |||
| if cfg.MaxRingSize > envconfig.RingHashCap { | |||
| cfg.MaxRingSize = envconfig.RingHashCap | |||
| } | |||
| if cfg.RequestMetadataKey != "" { | |||
| if err := metadata.ValidatePair(cfg.RequestMetadataKey, ""); err != nil { | |||
There was a problem hiding this comment.
Per A76:
If the request_hash_header refers to a binary header (suffixed with -bin), the configuration is also rejected.
Something's off here, ValidatePair currently does not err for keys suffixed with -bin. Should we add that additional validation here?
There was a problem hiding this comment.
optional: Also, for readability could we maybe split ValidatePair to md.ValidateKey and md.ValidateValue methods and call just ValidateKey from here so that we dont pass in a "" here?
| }, | ||
| { | ||
| name: "invalid request metadata keys", | ||
| js: `{"request_metadata_key": "!invalid"}`, |
There was a problem hiding this comment.
Let's add a case rejecting key referencing a binary header please
| } | ||
|
|
||
| // nextReadySubConn returns the first entry after e that has its | ||
| // subconn in READY state. If no such entry is found, a PickResult with a |
There was a problem hiding this comment.
seems incomplete. Did you mean?
| // subconn in READY state. If no such entry is found, a PickResult with a | |
| // subconn in READY state. If no such entry is found, returns nil. |
| for i := range p.ring.items { | ||
| e := p.ring.items[(e.idx+i)%len(p.ring.items)] | ||
| if e.sc.state == connectivity.Ready { | ||
| return e.sc.sc | ||
| } | ||
| } | ||
| return nil |
There was a problem hiding this comment.
How about we follow a similar pattern and do something like that:
| for i := range p.ring.items { | |
| e := p.ring.items[(e.idx+i)%len(p.ring.items)] | |
| if e.sc.state == connectivity.Ready { | |
| return e.sc.sc | |
| } | |
| } | |
| return nil | |
| for next := p.ring.next(e); next != e; next = ring.next(next) { | |
| if next.sc.state == connectivity.Ready { | |
| return next.sc.sc | |
| } | |
| } | |
| return nil |
There was a problem hiding this comment.
Yes, that works. I changed the code following your suggestion.
| // handleRICSResult is the return type of handleRICS. It's needed to wrap the | ||
| // returned error from Pick() in a struct. With this, if the return values are | ||
| // `balancer.PickResult, error, bool`, linter complains because error is not the | ||
| // last return value. | ||
| // returned error from Pick() in a struct and whether we triggered a connection | ||
| // attempt. Without this, the return values would be `balancer.PickResult, bool, error, bool`, | ||
| // and linter would complain because error is not the last return value. |
There was a problem hiding this comment.
line wrap please :)
| // handleRICSResult is the return type of handleRICS. It's needed to wrap the | |
| // returned error from Pick() in a struct. With this, if the return values are | |
| // `balancer.PickResult, error, bool`, linter complains because error is not the | |
| // last return value. | |
| // returned error from Pick() in a struct and whether we triggered a connection | |
| // attempt. Without this, the return values would be `balancer.PickResult, bool, error, bool`, | |
| // and linter would complain because error is not the last return value. | |
| // handleRICSResult is the return type of handleRICS. It's needed to wrap the | |
| // returned error from Pick() in a struct and whether we triggered a connection | |
| // attempt. Without this, the return values would be `balancer.PickResult, bool, | |
| // error, bool`, and linter would complain because error is not the last return | |
| // value. |
| @@ -84,23 +93,45 @@ func (p *picker) handleRICS(e *ringEntry) (handleRICSResult, bool) { | |||
| } | |||
|
|
|||
| func (p *picker) Pick(info balancer.PickInfo) (balancer.PickResult, error) { | |||
| e := p.ring.pick(getRequestHash(info.Ctx)) | |||
| h, usingRandomHash := getRequestHash(info.Ctx, p.requestHashKey) | |||
There was a problem hiding this comment.
I would prefer the name usesRandomHash
There was a problem hiding this comment.
Also if getRequestHash is made a method with the pointer receiver, then you could actually merge the logic to assign h = p.randunit64 inside getRequestHash? wdyt?
There was a problem hiding this comment.
I would prefer the name
usesRandomHash
I took your suggestion, but I was trying to follow Mark's suggested wording from the gRFC. I don't think there's ambiguity so I'm fine either way.
Also if getRequestHash is made a method with the pointer receiver, then you could actually merge the logic to assign h = p.randunit64 inside getRequestHash? wdyt?
I also took this suggestion. I removed the "ForTesting" suffix from GetXDSRequestHash, since it's now used inside picker.getRequestHash. It doesn't seem like a big deal, since all those APIs are within xds/internal.
|
This PR is labeled as requiring an update from the reporter, and no update has been received after 6 days. If no update is provided in the next 7 days, this issue will be automatically closed. |
|
You can hold on reviewing while I investigate making pick first the leaf policy for ring hash, as suggested in grpc/proposal#412 (comment). |
Implement part 1 of A76: the ability to set the request hash key, to extract the hash from a header. This allows using ring hash without xDS.
RELEASE NOTES:
request_metadata_keyload balancing config field (gRFC A76).