xds: wrr with random #2745
xds: wrr with random #2745
Conversation
menghanl
force-pushed
the
drop_wrr_random
branch
from
f21f40a
to
a680b04
Compare
balancer/internal/wrr/wrr.go
Outdated
|
|
||
| package wrr | ||
|
|
||
| // WRR blah |
There was a problem hiding this comment.
It's better to document there what is wrr.
| if rw.sumOfWeights == 0 { | ||
| return nil | ||
| } | ||
| randomWeight := grpcrand.Int63n(rw.sumOfWeights) |
There was a problem hiding this comment.
Make a comment saying that grpcrand.Int63n generates an integer [0, rw.sumOfweights).
| } | ||
| } | ||
|
|
||
| return rw.items[len(rw.items)-1].Item |
There was a problem hiding this comment.
I think this is not necessary. The for loop will cover all the cases.
There was a problem hiding this comment.
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.
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.
| // | ||
| // 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.
You can reuse the randWeighted struct once you change the name.
There was a problem hiding this comment.
They are in different packages.
| ) | ||
|
|
||
| // testWRR is a deterministic WRR implementation. |
There was a problem hiding this comment.
Can you explain in the comment why we need the deterministic WRR implementation for testing?
menghanl
force-pushed
the
drop_wrr_random
branch
from
8b1a2cc
to
c4958c0
Compare
| ) | ||
|
|
||
| // testWRR is a deterministic WRR implementation. With this, the balancer |
There was a problem hiding this comment.
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.
| package wrr | ||
|
|
||
| // WRR defines an interface that implements weighted round robin. | ||
| type WRR interface { |
There was a problem hiding this comment.
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.
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