-
Notifications
You must be signed in to change notification settings - Fork 4.6k
ringhash: move out of the xds package #8249
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
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #8249 +/- ##
==========================================
+ Coverage 82.02% 82.16% +0.13%
==========================================
Files 412 412
Lines 40477 40477
==========================================
+ Hits 33201 33256 +55
+ Misses 5897 5864 -33
+ Partials 1379 1357 -22
🚀 New features to boost your workflow:
|
This test failure is in a new ringhash test, but probably not related to this PR: https://github.com/grpc/grpc-go/actions/runs/14454284376/job/40533854683?pr=8249 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, adding a second reviewer.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please move this package under an internal
directory. Or can we move it into the parent directory and just put it in the ringhash_test
package?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Curious: is setting the xds request hash something that anyone else would need to be able to do? E.g. for testing? I'm fine with leaving this as-is for now, but I'm also OK with making that functionality public if we think it might be needed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The main problem here is a cross-language concern raised by Mark: see grpc/grpc#33356 (comment):
However, I am a little concerned about the complexity involved in exposing an interface to programmatically set the request's hash. When we discussed this earlier, we were thinking that this would probably just be something simple like "use a specific request header", which could just be built into the ring_hash policy. Exposing an API to allow applications to do this programmatically is a much broader API surface, and before going down that road, I'd like to be convinced that there's no simpler way to do this. Is there a way that we can handle your use-case without that complexity?
So I think that unless we are willing to provide functionalities that may not be available in other languages, it's better to limit the visibility to the xds use case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
SG. Sounds like users won't need to be able to set it this way for any testing, either, so let's keep it as is.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
fixes #8213
There is a new API in that ring hash can now be used by importing
google.golang.org/grpc/balancer/ringhash
instead of importing all of
google.golang.org/grpc/xds
.RELEASE NOTES: