-
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
Changes from all commits
53e33db
fad5043
d77824f
0c70d5f
f11e5ec
9850c62
fcb660f
68320d3
77384ae
0a9c51d
57ee0d2
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,82 @@ | ||
/* | ||
* | ||
* Copyright 2020 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 weightedroundrobin | ||
|
||
import ( | ||
"testing" | ||
|
||
"github.com/google/go-cmp/cmp" | ||
"google.golang.org/grpc/attributes" | ||
"google.golang.org/grpc/resolver" | ||
) | ||
|
||
func TestAddrInfoToAndFromAttributes(t *testing.T) { | ||
tests := []struct { | ||
desc string | ||
inputAddrInfo AddrInfo | ||
inputAttributes *attributes.Attributes | ||
wantAddrInfo AddrInfo | ||
}{ | ||
{ | ||
desc: "empty attributes", | ||
inputAddrInfo: AddrInfo{Weight: 100}, | ||
inputAttributes: nil, | ||
wantAddrInfo: AddrInfo{Weight: 100}, | ||
}, | ||
{ | ||
desc: "non-empty attributes", | ||
inputAddrInfo: AddrInfo{Weight: 100}, | ||
inputAttributes: attributes.New("foo", "bar"), | ||
wantAddrInfo: AddrInfo{Weight: 100}, | ||
}, | ||
{ | ||
desc: "addrInfo not present in empty attributes", | ||
inputAddrInfo: AddrInfo{}, | ||
inputAttributes: nil, | ||
wantAddrInfo: AddrInfo{}, | ||
}, | ||
{ | ||
desc: "addrInfo not present in non-empty attributes", | ||
inputAddrInfo: AddrInfo{}, | ||
inputAttributes: attributes.New("foo", "bar"), | ||
wantAddrInfo: AddrInfo{}, | ||
}, | ||
} | ||
|
||
for _, test := range tests { | ||
t.Run(test.desc, func(t *testing.T) { | ||
addr := resolver.Address{Attributes: test.inputAttributes} | ||
addr = SetAddrInfo(addr, test.inputAddrInfo) | ||
gotAddrInfo := GetAddrInfo(addr) | ||
if !cmp.Equal(gotAddrInfo, test.wantAddrInfo) { | ||
t.Errorf("gotAddrInfo: %v, wantAddrInfo: %v", gotAddrInfo, test.wantAddrInfo) | ||
} | ||
|
||
}) | ||
} | ||
} | ||
|
||
func TestGetAddInfoEmpty(t *testing.T) { | ||
addr := resolver.Address{Attributes: attributes.New()} | ||
gotAddrInfo := GetAddrInfo(addr) | ||
wantAddrInfo := AddrInfo{} | ||
if !cmp.Equal(gotAddrInfo, wantAddrInfo) { | ||
t.Errorf("gotAddrInfo: %v, wantAddrInfo: %v", gotAddrInfo, wantAddrInfo) | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -277,9 +277,16 @@ func (edsImpl *edsBalancerImpl) handleEDSResponsePerPriority(bgwc *balancerGroup | |
Addr: lbEndpoint.Address, | ||
} | ||
if edsImpl.subBalancerBuilder.Name() == weightedroundrobin.Name && lbEndpoint.Weight != 0 { | ||
address.Metadata = &weightedroundrobin.AddrInfo{ | ||
Weight: lbEndpoint.Weight, | ||
} | ||
ai := weightedroundrobin.AddrInfo{Weight: lbEndpoint.Weight} | ||
address = weightedroundrobin.SetAddrInfo(address, ai) | ||
// Metadata field in resolver.Address is deprecated. The | ||
// attributes field should be used to specify arbitrary | ||
// 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 commentThe 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 commentThe 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 commentThe 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 commentThe 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 commentThe 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 commentThe 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 commentThe 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 commentThe 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 commentThe reason will be displayed to describe this comment to others. Learn more. Issue filed #3563 |
||
// See https://github.com/grpc/grpc-go/issues/3563. | ||
address.Metadata = &ai | ||
} | ||
newAddrs = append(newAddrs, 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.
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 whenv
isnil
.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.
https://play.golang.org/p/-DTj1hwQlPh
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.