services, xds, orca: support EPS in client-side WRR #10177
Conversation
|
@YifeiZhuang PTAL. Thanks. |
danielzhaotongliu
force-pushed
the
eps-support
branch
from
32dce41 to
0e2f7e6
Compare
xds/src/main/java/io/grpc/xds/WeightedRoundRobinLoadBalancer.java
Outdated
Show resolved
Hide resolved
xds/src/main/java/io/grpc/xds/WeightedRoundRobinLoadBalancer.java
Outdated
Show resolved
Hide resolved
Were you able to request a reviewer if you are not a maintainer? The PR is mostly fine to me. Only a few comments. |
No. I am not able to request any reviewer in any opened PR (that option is greyed-out for me) since I am not a maintainer |
| @@ -172,8 +173,8 @@ public Subchannel createSubchannel(CreateSubchannelArgs args) { | |||
| @VisibleForTesting | |||
| final class WrrSubchannel extends ForwardingSubchannel { | |||
| private final Subchannel delegate; | |||
| private final OrcaOobReportListener oobListener = this::onLoadReport; | |||
| private final OrcaPerRequestReportListener perRpcListener = this::onLoadReport; | |||
| private final OrcaOobReportListener oobListener = new OrcaReportListener( | |||
There was a problem hiding this comment.
Here the oobListener should not be constructed like this - because it won't get the updated config for the whole lifetime of the subchannel. (as we talked before, each address update in the LB with acceptResolvedAddresses, some subchannels can be reused if their address is the same).
For the optimized oobListener (avoid creating frequently), it should use the config object directly in onLoadReport(so that it uses the updated config and it is also thread safe because onLoadReport is called within syncContext), but not constructed with the current config.errorUtilizationPenalty value.
Since the oobListener is set per each address update worse case, not as frequent as pick subchannel, you can just create a new ooblistener each time needed.
There was a problem hiding this comment.
Done.
Created a new OOB listener each time needed.
Thanks for the detailed context and explanation, I previously thought the OOB listener is completely separated from the lifecycle of channel & picker.
| @@ -251,7 +269,7 @@ public PickResult pickSubchannel(PickSubchannelArgs args) { | |||
| if (!enableOobLoadReport) { | |||
| return PickResult.withSubchannel(subchannel, | |||
| OrcaPerRequestUtil.getInstance().newOrcaClientStreamTracerFactory( | |||
| ((WrrSubchannel)subchannel).perRpcListener)); | |||
| ((WrrSubchannel)subchannel).new OrcaReportListener(errorUtilizationPenalty))); | |||
There was a problem hiding this comment.
Here the per-request listener can be created once at picker's construction time and reused.
A followup PR is fine.
There was a problem hiding this comment.
Done.
The per-request listener is created once (per channel & tracked with a map) in the picker's constructor.
I added a "fallback" case with getOrDefault for safety.
YifeiZhuang
added
the
kokoro:force-run
grpc-kokoro
removed
the
kokoro:force-run
Design: go/wrr-eps-design-review-slides
Uses: #10160
Tested: updated & ran tests 100 times to ensure they are not flaky