-
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
xds: wrr with random #2745
xds: wrr with random #2745
Conversation
f21f40a
to
a680b04
Compare
balancer/internal/wrr/wrr.go
Outdated
|
||
package wrr | ||
|
||
// WRR blah |
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's better to document there what is wrr.
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 rw.sumOfWeights == 0 { | ||
return nil | ||
} | ||
randomWeight := grpcrand.Int63n(rw.sumOfWeights) |
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 comment saying that grpcrand.Int63n generates an integer [0, rw.sumOfweights).
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
} | ||
} | ||
|
||
return rw.items[len(rw.items)-1].Item |
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 think this is not necessary. The for loop will cover all the cases.
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.
Compiler will think this is missing a return.
balancer/internal/wrr/random.go
Outdated
import "google.golang.org/grpc/internal/grpcrand" | ||
|
||
// randWeighted is a wrapped weighted item that is used to implement weighted random algorithm. | ||
type randWeighted struct { |
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.
can we remove the "rand" prefix, and name it e.g. weightedItem
? The struct itself is unrelated to randomness, it's just you currently only used it for the randomWRR
.
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
// | ||
// With {a: 2, b: 3}, the Next() results will be {a, a, b, b, b}. | ||
type testWRR struct { | ||
itemsWithWeight []struct { |
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.
You can reuse the randWeighted
struct once you change the name.
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.
They are in different packages.
) | ||
|
||
// testWRR is a deterministic WRR implementation. |
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.
Can you explain in the comment why we need the deterministic WRR implementation for testing?
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
8b1a2cc
to
c4958c0
Compare
) | ||
|
||
// testWRR is a deterministic WRR implementation. With this, the 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.
How about adding that the real implementation is non-deterministic, and it is covered by unit testing. And here, we use the deterministic implementation to test logic beyond the non-deterministic part.
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
package wrr | ||
|
||
// WRR defines an interface that implements weighted round robin. | ||
type WRR interface { |
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.
Does WRR need to be thread safe? Or if there's a guarantee that Add or Next won't be called concurrently. It's better to document here clearly.
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.
Next() needs to be thread safe.
Add() I'm not sure. I'm leaving it undocumented. Will add more requirements when adding new implementations.
for drops and inter-locality picks