-
Notifications
You must be signed in to change notification settings - Fork 4.6k
grpc: do not use balancer attributes during address comparison #6439
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
grpc: do not use balancer attributes during address comparison #6439
Conversation
attributes/attributes.go
Outdated
| if str, ok := k.(interface{ String() string }); ok { | ||
| key = str.String() | ||
| } else if str, ok := k.(string); ok { | ||
| key = str | ||
| } |
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.
Sad that string doesn't implement Stringer 😞
To consolidate these code paths, and to avoid printing literally ", " if key & val are both not string-y, maybe we should have something like this?:
func str(x interface{}) string {
if v, ok := x.(fmt.Stringer); ok {
return v.String()
} else if v, ok := x.(string); ok {
return v
}
return fmt.Sprintf("<%p>", x)
}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. Had to add a "%q" format string to ensure that the key and value strings are quoted.
| Network string | ||
| Address string | ||
| Target string | ||
| Listener net.Listener |
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.
This is not a parameter for Listen or Dial. Can you document this please and indicate Network & Address aren't used if this is set, and that Target is the responsibility of the user to set properly when Listener is used?
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.
My code search reveals that nobody is setting Network, Address or Target.
Address is used as a read-only field by numerous tests to retrieve the address of the stub server. Should we just get rid of Network and Target and make Address as a read-only field, or make it a getter method? (Can do in a follow up PR)
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.
SGTM!
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.
Will do the cleanup in a follow up PR. Thanks.
| } else if v, ok := x.(string); ok { | ||
| return fmt.Sprintf("%q", v) | ||
| } | ||
| return fmt.Sprintf("<%p>", x) |
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.
This one doesn't print literal quotes.
Why not keep it the way it was? Have this return the actual string and quote it when it's used?
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.
Yeah, makes sense.
Summary of changes:
net.Listenerto thestubserverFixes internal issue: b/290272899
RELEASE NOTES: