-
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
xdsrouting: resolver to generate service config with routes, and pick routing balancer #3751
Conversation
menghanl
commented
Jul 20, 2020
•
edited
Loading
edited
…s, and pick routing as the top level balancer
b73ce8b
to
252727d
Compare
This PR is rebased on top of the balancer, and tests are passing. Should be ready for review. Thanks! |
252727d
to
044dbc8
Compare
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.
Need to make another pass through the tests.
oldNextIndex map[string]int | ||
newActions map[string]action | ||
wantActions map[string]action | ||
wantNextIndex map[string]int |
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.
Testing the value of wantNextIndex
seems to be testing implementation rather than behavior. And if we go with my other suggestion of using random numbers for the next index, we wouldn't have to test that at all.
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 to make sure that the indices are incrementing, so it won't generate the same assigned names.
Random number would make this, and the expected assigned names hard to test.
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 for the review!
|
||
// Find actions in newActions but not in oldActions. Add them, and try to | ||
// reuse assigned names from actionsRemoved. | ||
if r.nextIndex == 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.
I'm not too worried about overflow, and I made it a uint64
now.
Random number would work, but it makes the tests harder.
oldNextIndex map[string]int | ||
newActions map[string]action | ||
wantActions map[string]action | ||
wantNextIndex map[string]int |
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 to make sure that the indices are incrementing, so it won't generate the same assigned names.
Random number would make this, and the expected assigned names hard to test.
I sent a comment about trying to override the random generator with a monotonic generator in tests. But not sure why it doesn't show up here. :( |