-
Notifications
You must be signed in to change notification settings - Fork 4.7k
ringhash: implement gRFC A76 #8159
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
Changes from 3 commits
5f56c4c
db645b6
2bfd696
132b401
8ac641b
54caf30
90a6ff4
ee20ac2
c0b77ad
3fa56e9
febd62a
8816ae5
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
|
arjan-bal marked this conversation as resolved.
|
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,59 @@ | ||
| /* | ||
| * | ||
| * Copyright 2025 gRPC authors. | ||
| * | ||
| * Licensed under the Apache License, Version 2.0 (the "License"); | ||
| * you may not use this file except in compliance with the License. | ||
| * You may obtain a copy of the License at | ||
| * | ||
| * http://www.apache.org/licenses/LICENSE-2.0 | ||
| * | ||
| * Unless required by applicable law or agreed to in writing, software | ||
| * distributed under the License is distributed on an "AS IS" BASIS, | ||
| * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
| * See the License for the specific language governing permissions and | ||
| * limitations under the License. | ||
| * | ||
| */ | ||
|
|
||
| // Package ringhash implements resolver related functions for the ring_hash | ||
| // load balancing policy. | ||
| package ringhash | ||
|
|
||
| import ( | ||
| "google.golang.org/grpc/resolver" | ||
| ) | ||
|
|
||
| type hashKeyType string | ||
|
|
||
| // hashKeyKey is the key to store the ring hash key attribute in | ||
| // a resolver.Endpoint attribute. | ||
| const hashKeyKey = hashKeyType("hash_key") | ||
|
easwars marked this conversation as resolved.
Outdated
|
||
|
|
||
| // SetEndpointHashKey sets the hash key for this endpoint. Combined with the | ||
| // ring_hash load balancing policy, it allows placing the endpoint on the ring | ||
| // based on an arbitrary string instead of the IP address. | ||
|
easwars marked this conversation as resolved.
Outdated
|
||
| // | ||
| // # Experimental | ||
| // | ||
| // Notice: This API is EXPERIMENTAL and may be changed or removed in a | ||
| // later release. | ||
| func SetEndpointHashKey(ep resolver.Endpoint, hashKey string) resolver.Endpoint { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: We can remove
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. +1
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nit: s/hash key attribute of addr/hash key attribute of endpoint/ |
||
| if hashKey == "" { | ||
| return ep | ||
| } | ||
| ep.Attributes = ep.Attributes.WithValue(hashKeyKey, hashKey) | ||
| return ep | ||
| } | ||
|
|
||
| // GetEndpointHashKey returns the hash key attribute of addr. If this attribute | ||
| // is not set, it returns the empty string. | ||
| // | ||
| // # Experimental | ||
| // | ||
| // Notice: This API is EXPERIMENTAL and may be changed or removed in a | ||
| // later release. | ||
| func GetEndpointHashKey(ep resolver.Endpoint) string { | ||
|
easwars marked this conversation as resolved.
Outdated
|
||
| hashKey, _ := ep.Attributes.Value(hashKeyKey).(string) | ||
| return hashKey | ||
| } | ||
|
arjan-bal marked this conversation as resolved.
|
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -26,6 +26,10 @@ import ( | |
| ) | ||
|
|
||
| func (s) TestParseConfig(t *testing.T) { | ||
| oldEnvConfig := envconfig.RingHashSetRequestHashKey | ||
| defer func() { envconfig.RingHashSetRequestHashKey = oldEnvConfig }() | ||
| envconfig.RingHashSetRequestHashKey = true | ||
|
|
||
| tests := []struct { | ||
| name string | ||
| js string | ||
|
|
@@ -94,6 +98,27 @@ func (s) TestParseConfig(t *testing.T) { | |
| want: nil, | ||
| wantErr: true, | ||
| }, | ||
| { | ||
| name: "request metadata key set", | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. there should be a test case for when there are multiple header values from the gRFC A76
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The behavior joining headers with a coma is for header values. Using multiple headers is not supported. I added a picker test case for this, and took the opportunity to make the picker test a bit more robust by distributing endpoints on the ring, and removing unnecessary setting of the request hash in existing tests.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can we have a test for the case where the env var is set to
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Ack. Thanks for adding this test.
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Added.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nit: Since we have multiple env vars in the context of ring_hash config parsing, could we please rename the field something more specific. Maybe |
||
| js: `{"request_hash_header": "x-foo"}`, | ||
| want: &LBConfig{ | ||
| MinRingSize: defaultMinSize, | ||
| MaxRingSize: defaultMaxSize, | ||
| RequestHashHeader: "x-foo", | ||
| }, | ||
| }, | ||
| { | ||
| name: "invalid request hash header", | ||
| js: `{"request_hash_header": "!invalid"}`, | ||
| want: nil, | ||
| wantErr: true, | ||
| }, | ||
| { | ||
| name: "binary request hash header", | ||
| js: `{"request_hash_header": "header-with-bin"}`, | ||
| want: nil, | ||
| wantErr: true, | ||
| }, | ||
| } | ||
| for _, tt := range tests { | ||
| t.Run(tt.name, func(t *testing.T) { | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.