xds/internal/xdsclient: Add least request support in xDS #6517
Conversation
|
Tests are still unhappy. |
zasweq
force-pushed
the
least-request-in-xds
branch
2 times, most recently
from
cf9e1f5
to
e5f50f2
Compare
|
I think should be good to review. Sorry for missing that haha. |
|
Got to all comments. Thanks for the pass :D. |
zasweq
force-pushed
the
least-request-in-xds
branch
4 times, most recently
from
c031b2f
to
8684203
Compare
|
Could you please resolve the conflicts before I take another pass. Thanks. |
zasweq
force-pushed
the
least-request-in-xds
branch
from
8684203
to
b8ac621
Compare
|
Done. The thing that created conflicts was the deexperimentalization of pick first. |
| ChoiceCount: wrapperspb.UInt32(2), | ||
| }), | ||
| // Least request's randomness of indexes to sample (unary RPCs which | ||
| // don't persist so never any actual RPC counts over iterations) |
There was a problem hiding this comment.
Something is incomplete in this sentence:
unary RPCs which don't persist so never any actual RPC counts over iterations
There was a problem hiding this comment.
Changed to "The test performs a Unary RPC, and blocks until the RPC returns, and then makes the next RPC. Thus, over iterations, no RPC counts are present. This causes Least request's randomness of indexes to sample to converge onto a round robin distribution per locality. Thus, expect the same distribution as round robin above."
| // "The configuration for the Least Request LB policy is the | ||
| // least_request_lb_config field. The field is optional; if not present, | ||
| // defaults will be assumed for all of its values." - A48 | ||
| var choiceCount uint32 = defaultLeastRequestChoiceCount |
There was a problem hiding this comment.
Not asking you to change, but just FYI choiceCount := uint32(defaultLeastRequestChoiceCount) is shorter.
There was a problem hiding this comment.
Switched. I feel like throughout the codebase and my years on the team, x := y vs. var x type = y has always been preferred for local vars declaration to a non zero value. Noted for future.
| if test.lrDisabled { | ||
| defer func(old bool) { envconfig.LeastRequestLB = old }(envconfig.LeastRequestLB) | ||
| envconfig.LeastRequestLB = false | ||
| } |
There was a problem hiding this comment.
Default value of envconfig.LeastRequestLB is already false. You are setting it to true for the duration of the test at the top, and then setting it to false here again.
Why not have a lrEnabled field which is set to true only for tests that require it, and here, you can check for if test.lrEnabled { // set the env var to true }
There was a problem hiding this comment.
Done :). Good suggestion.
| if cc := lr.GetChoiceCount(); cc != nil { | ||
| choiceCount = cc.GetValue() | ||
| } | ||
| // nack if it's < 2, and also add test for it |
There was a problem hiding this comment.
Please remove the also add test for it part of it.
And probably quote the relevant language from the gRFC if that makes sense.
There was a problem hiding this comment.
Switched comment to: // "If choice_count < 2, the config will be rejected." - A48
This PR adds least request support plumbed through xDS.
RELEASE NOTES: