-
Notifications
You must be signed in to change notification settings - Fork 4.4k
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
Pass weights to wrr balancer through attributes. #3530
Conversation
"testing" | ||
|
||
"github.com/google/go-cmp/cmp" | ||
|
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.
Empty line
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.
Intellij seems to be doing this. I have been trying to delete them wherever I notice.
|
||
// Attribute key used to store AddrInfo in the Attributes field of | ||
// resolver.Address. | ||
attributeKey = "/balancer/weightedroundrobin/addrInfo" |
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.
Make a type, so only this package can get/set the value?
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. Thanks.
|
||
// AddAddrInfoToAttributes returns a new Attributes containing all key/value | ||
// pairs in a with ai being added to it. | ||
func AddAddrInfoToAttributes(ai *AddrInfo, a *attributes.Attributes) *attributes.Attributes { |
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.
Should this take an address and return an address?
And this is not "add", this is "set", because it overrides the previous info, if there's one.
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.
type AddrInfo struct { | ||
Weight uint32 | ||
} | ||
|
||
// SetAddrInfo sets addInfo in the Attributes field of addr. | ||
func SetAddrInfo(addrInfo *AddrInfo, addr *resolver.Address) { |
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.
There's a discussion on whether the function should take a pointer and modify, or make a copy: #3494 (comment). Let's wait for that.
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.
And make the function experimental.
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 change these to not be pointers - do copies instead. Since these are not performance sensitive, we will optimize for code clarity and proper usage, which copying would better provide, I believe. E.g. using a pointer means modifications to the AddrInfo
passed in will affect addresses which received a pointer to it in their attributes. This could lead to bugs.
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.
attributes/attributes.go
Outdated
@@ -50,6 +50,9 @@ func New(kvs ...interface{}) *Attributes { | |||
// times, the last value overwrites all previous values for that key. To | |||
// remove an existing key, use a nil value. | |||
func (a *Attributes) WithValues(kvs ...interface{}) *Attributes { | |||
if a == nil { | |||
a = New() |
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.
return New(kvs)
.
Note that Value
doesn't handle a nil
a
.
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 for both.
type AddrInfo struct { | ||
Weight uint32 | ||
} | ||
|
||
// SetAddrInfo sets addInfo in the Attributes field of addr. | ||
func SetAddrInfo(addrInfo *AddrInfo, addr *resolver.Address) { |
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 change these to not be pointers - do copies instead. Since these are not performance sensitive, we will optimize for code clarity and proper usage, which copying would better provide, I believe. E.g. using a pointer means modifications to the AddrInfo
passed in will affect addresses which received a pointer to it in their attributes. This could lead to bugs.
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.
PTAL.
type AddrInfo struct { | ||
Weight uint32 | ||
} | ||
|
||
// SetAddrInfo sets addInfo in the Attributes field of addr. | ||
func SetAddrInfo(addrInfo *AddrInfo, addr *resolver.Address) { |
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.
attributes/attributes.go
Outdated
@@ -50,6 +50,9 @@ func New(kvs ...interface{}) *Attributes { | |||
// times, the last value overwrites all previous values for that key. To | |||
// remove an existing key, use a nil value. | |||
func (a *Attributes) WithValues(kvs ...interface{}) *Attributes { | |||
if a == nil { | |||
a = New() |
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 for both.
type AddrInfo struct { | ||
Weight uint32 | ||
} | ||
|
||
// SetAddrInfo sets addInfo in the Attributes field of addr. |
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.
"sets addrInfo"
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.
|
||
// SetAddrInfo sets addInfo in the Attributes field of addr. | ||
// This is an EXPERIMENTAL API. | ||
func SetAddrInfo(addrInfo AddrInfo, addr *resolver.Address) { |
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 update this to return a resolver.Address
and not accept a pointer.
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.
// GetAddrInfo returns the AddrInfo stored in the Attributes fields of addr. | ||
// Returns nil if no AddrInfo is present. | ||
// This is an EXPERIMENTAL API. | ||
func GetAddrInfo(addr *resolver.Address) AddrInfo { |
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.
Also, don't accept a pointer here for symmetry and simplicity. Performance is not a concern here.
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.
// attributes about the address. We still need to populate the | ||
// Metadata field here to allow users of this field to migrate | ||
// to the new one. | ||
// TODO(easwars): Remove this once all users have migrated. |
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.
Is this behavior documented? Who are the users you're referring to? Only us?
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.
@menghanl mentioned that dropbox was a user.
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.
I'm a little lost. This is the address we're passing to the weighted round robin balancer - why do we need to set Metadata for it? Are they using xds and overriding our weighted RR balancer?
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.
I'm not very sure how (or for what) they use it. @menghanl might have an answer.
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.
Yes. And they use edf wrr #2968
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.
@menghanl How will we track the safe removal of this?
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.
Keep it for one more release, and remove after 1.30?
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.
That's fine with me if it's OK with dropbox. Please file an issue and cc them?
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.
Issue filed #3563
attributes/attributes.go
Outdated
// Clone returns a new Attributes containing all key/value pairs found in a. | ||
// Since Attributes stores its key/value paris as type `interface{}`, it is not | ||
// possible to perform deep copies here. | ||
func (a *Attributes) Clone() *Attributes { |
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.
Attributes are immutable, so if we can't do a deep copy, I don't think there's any value of a Clone()
, is there?
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.
WithValues()
is already making a clone.
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.
I guess what I wanted to say was that since Attributes
stores key/value pairs of type interface{}
, one can store a pointer as the value type. The caller of the Clone
operation can then modify the contents of the data pointed to by the value. This is no different from what WithValues
does. So, maybe I should remove that comment about the deep copy.
In fact I could and maybe i should replace the implementation of Clone
as follows:
func (a *Attributes) Clone() *Attributes {
return a.WithValues()
}
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.
But why have Clone
at all? Or call Attributes.WithValues
with no parameters? There is no way to modify a *Attributes
, so there is no need to Clone
it. I.e.:
a := attributes.New(kvs...)
b := a.WithValues(otherKVs...)
c := a.WithValues(otherOtherKVs...)
a
, b
, and c
will all have kvs
, b
will be the only thing with otherKVs
, and c
will be the only one with otherOtherKVs
. You can safely pass around and make copies of *Attributes
.
If a user stores a pointer in *Attributes
and modifies it later, Clone
-ing the *Attributes
won't change the fact that modification to that pointer will modify the *Attributes
(clones and the original). The only way to prevent that would have been to force the values stored in Attributes
to have a Clone
method on them, and call that in WithValues
/Clone
.
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.
Makes sense, got rid of Clone
.
Ping ... |
// | ||
// This is an EXPERIMENTAL API. | ||
func SetAddrInfo(addrInfo AddrInfo, addr resolver.Address) resolver.Address { | ||
newAddr := addr |
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.
Since this is passed by value, there's no need to make this copy here.
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.
// | ||
// This is an EXPERIMENTAL API. | ||
func GetAddrInfo(addr resolver.Address) AddrInfo { | ||
if addr.Attributes == nil { |
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.
Value
handles nil
*Attributes
, so this check can be removed.
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.
if addr.Attributes == nil { | ||
return AddrInfo{} | ||
} | ||
ai := addr.Attributes.Value(attributeKey{}) | ||
if ai == nil { | ||
return AddrInfo{} | ||
} | ||
if _, ok := ai.(AddrInfo); !ok { | ||
return AddrInfo{} | ||
} | ||
return ai.(AddrInfo) |
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.
Since everything is nil-pointer-safe, I think this function can be simplified to something like:
v := addr.Attributes.Value(attributesKey{})
ai, _ := v.(AddrInfo) // ignore ok and use zero value AddrInfo if v is nil
return ai
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. Thanks.
// attributes about the address. We still need to populate the | ||
// Metadata field here to allow users of this field to migrate | ||
// to the new one. | ||
// TODO(easwars): Remove this once all users have migrated. |
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.
I'm a little lost. This is the address we're passing to the weighted round robin balancer - why do we need to set Metadata for it? Are they using xds and overriding our weighted RR balancer?
// with addrInfo. | ||
// | ||
// This is an EXPERIMENTAL API. | ||
func SetAddrInfo(addrInfo AddrInfo, addr resolver.Address) resolver.Address { |
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.
Nit: should addr
come before addrInfo
?
So this looks more like a method on resolver.Address
, which it probably should be but we cannot do.
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.
} | ||
|
||
// GetAddrInfo returns the AddrInfo stored in the Attributes fields of addr. | ||
// Returns nil if no AddrInfo is present. |
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 returning nil.
Should we just return the zero value? Or also return an OK?
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.
It would be better if we can change Attributes.Value
to return (interface{}, bool)
. So, all users of that package will benefit.
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.
I don't understand - can you explain how this will help?
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.
Given that the key for Attributes
is local to the package, if Attributes.Value
returned an extra return parameter indicating whether or not it found the key, the callers could be less paranoid when type asserting the returned interface{}
and simply return a default value if the second return parameter is false
.
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 type assertion is safe, however, if you use the , ok
form of it. And you'll get the zero value if the interface{}
is nil
. And if you do need it, you can rewrite:
v, ok := a.Value(...)
if ok {
as
v := a.Value(...)
if v != nil {
// This is an EXPERIMENTAL API. | ||
func GetAddrInfo(addr resolver.Address) AddrInfo { | ||
v := addr.Attributes.Value(attributeKey{}) | ||
ai, _ := v.(AddrInfo) |
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 panics if the info is not in attributes.
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. 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.
Are you sure? I don't think it will. The , _
should prevent the panic when v
is nil
.
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.
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.
[facepalm] Does it have , _
? My bad.
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.
Wanted to add a test and make sure, but was lazy. Done now.
PTAL. |
@dfawley : Please let me know if you have any more comments. Thanks. |
weightedroundrobin
package to add/getAddrInfo
to/fromattributes.Attributes
.attributes.WithValues
to work with a nil receiver to avoid call sites from having to perform nil checks and creating emptyAttributes
before being able to use this method.#effectively-use-attributes