balancer: expose endpoint weight and hostname as experimental APIs#9074
balancer: expose endpoint weight and hostname as experimental APIs#9074hpathak01 wants to merge 10 commits intogrpc:masterfrom
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #9074 +/- ##
==========================================
+ Coverage 80.52% 83.09% +2.56%
==========================================
Files 413 414 +1
Lines 33543 33480 -63
==========================================
+ Hits 27012 27819 +807
+ Misses 4316 4236 -80
+ Partials 2215 1425 -790
🚀 New features to boost your workflow:
|
…rpc#8971) - Add new public packages balancer/weight and balancer/hostname with Set/FromEndpoint and Set/Hostname/HostnameFromAddress helpers. - Mark all new APIs as # Experimental with the standard notice. - Update internal callers (ringhash, pickfirst, cdsbalancer, xdsresource) to use the public packages. - Remove dead hostnameKeyType from xdsresource and delegate legacy Hostname() to the public API (fixes Authority_Rewrite_Mismatch test). - Keep deprecated internal/balancer/weight package for transition. - Add tests for the new public packages. - Verified end-to-end with custom DAPerture balancer.
7e7f078 to
5ed3447
Compare
Yes, that is expected. |
|
@Pranjali-2501 : Could you please do a first pass. |
Pranjali-2501
left a comment
There was a problem hiding this comment.
Hi @hpathak01, thanks for raising the PR.
I have added some comments, please take a look.
| // | ||
| // Notice: This API is EXPERIMENTAL and may be changed or removed in a | ||
| // later release. | ||
| func Hostname(endpoint resolver.Endpoint) string { |
There was a problem hiding this comment.
I think it will be better to rename it as FromEndpoint.
| return h | ||
| } | ||
|
|
||
| // FromAddress returns the hostname attribute from a legacy |
There was a problem hiding this comment.
We don't need to export the function to retrieve hostname from address.attributes and address.BalancerAttributes.
Please remove this.
| "time" | ||
|
|
||
| "github.com/google/go-cmp/cmp" | ||
|
|
There was a problem hiding this comment.
Remove this new line.
| // Package weight contains utilities to manage endpoint weights. Weights are | ||
| // used by LB policies such as ringhash to distribute load across multiple | ||
| // endpoints. | ||
| // |
There was a problem hiding this comment.
Instead of putting a deprecated message and creating new files in balancer/weight/, use git move to move the files from internal/balancer/weight/weight.go to balancer/weight/weight.go.
There was a problem hiding this comment.
done - i also added the "Experimental" comment because this is a newly-public API. is that fine?
| * | ||
| */ | ||
|
|
||
| // Deprecated: use google.golang.org/grpc/balancer/weight instead. |
There was a problem hiding this comment.
Same as above, use git move to move this file also.
| "time" | ||
|
|
||
| "github.com/google/go-cmp/cmp" | ||
|
|
There was a problem hiding this comment.
Please remove this newline.
| } | ||
| endpoint.Attributes = endpoint.Attributes.WithValue(hostnameKeyType{}, hostname) | ||
| return endpoint | ||
| func SetHostname(endpoint resolver.Endpoint, h string) resolver.Endpoint { |
There was a problem hiding this comment.
We don't need to call exported Get/Set function from another function. Directly use hostname.Set whenever required.
| // Address. If this attribute is not set, it returns the empty string. | ||
| // Hostname returns the hostname from the given legacy Address. | ||
| // If this attribute is not set, it returns the empty string. | ||
| func Hostname(addr resolver.Address) string { |
There was a problem hiding this comment.
Please revert the changes made in this function. We have an exported function to set hostname in endpoint. And will keep the function internal to retrieve the hostname resolver.Address.
There was a problem hiding this comment.
hi @Pranjali-2501 reverting this function causes two issues:
- The original
hostnameKeyTypehas been removed above, so it wouldn't compile. - Even if we bring hostnameKeyType back, the test
TestAuthorityOverridingWithTLS/Authority_Rewrite_Mismatchfails — because EDS unmarshal now writes hostname viahostname.Set(which uses thehostnameKeydefined inbalancer/hostname/hostname.go), but the originalHostname(addr)would read with its own internal key type, so the hostname is never found.
The current implementation delegates to hostname.FromEndpoint so both setter and getter use the same key. All tests pass. Does this approach work, or would you prefer a different solution?
There was a problem hiding this comment.
IMO, the best way will be to move this function to balancer/hostname/hostname.go, export it and name it FromAddress.
| // Address. If this attribute is not set, it returns the empty string. | ||
| // Hostname returns the hostname from the given legacy Address. | ||
| // If this attribute is not set, it returns the empty string. | ||
| func Hostname(addr resolver.Address) string { |
There was a problem hiding this comment.
IMO, the best way will be to move this function to balancer/hostname/hostname.go, export it and name it FromAddress.
|
@hpathak01 , please fix Testing / static checks (latest-1) (pull_request). |
hi @Pranjali-2501 done |
| return endpoint | ||
| } | ||
|
|
||
| // FromEndpoint returns the hostname attribute of endpoint. If this attribute is |
There was a problem hiding this comment.
nit: wrap the comment in 80 columns.
|
Hi @easwars , just checking in — I've addressed all the review comments. Would you have a chance to take a look when you get a moment? Thanks! |
Description
This PR exposes two new experimental public APIs so that custom load balancing policies can access endpoint attributes that were previously only available via internal packages:
google.golang.org/grpc/balancer/weight—Set,FromEndpoint,EndpointInfogoogle.golang.org/grpc/balancer/hostname—Set,FromEndpoint,FromAddressThese attributes (especially
weight) are required by custom balancers such as deterministic aperture / P2C that need EDS-provided endpoint details like weights etc.Changes
balancer/hostnamewith full experimental notices.internal/balancer/weightto publicbalancer/weightvia git mv, added experimental notices.SetHostnameandHostnamewrappers fromxdsresource; callers now usehostname.Setandhostname.FromAddressdirectly.Related Issues
Fixes #8971
Testing
go test -count=1 ./...)Notes for Reviewers
resolver/ringhash.balancer/weightandbalancer/hostname).RELEASE NOTES: none