Skip to content

Commit a6f8ebf

Browse files
ejona86ejona86
committed
Remove implicit requestConnection() on IDLE from MultiChildLB
One LB no longer needs to extend ChildLbState and one has to start, so it is a bit of a wash. There are more LBs that need the auto-request logic, but if we have an API where subclasses override it without calling super then we can't change the implementation in the future. Adding behavior on top of a base class allows subclasses to call super, which lets the base class change over time.
1 parent 4ab3422 commit a6f8ebf

File tree

6 files changed

+48
-52
lines changed

6 files changed

+48
-52
lines changed

‎util/src/main/java/io/grpc/util/MultiChildLoadBalancer.java‎

Lines changed: 0 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -456,8 +456,6 @@ protected class ChildLbStateHelper extends ForwardingLoadBalancerHelper {
456456
/**
457457
* Update current state and picker for this child and then use
458458
* {@link #updateOverallBalancingState()} for the parent LB.
459-
*
460-
* <p/>Override this if you don't want to automatically request a connection when in IDLE
461459
*/
462460
@Override
463461
public void updateBalancingState(final ConnectivityState newState,
@@ -471,9 +469,6 @@ public void updateBalancingState(final ConnectivityState newState,
471469
// If we are already in the process of resolving addresses, the overall balancing state
472470
// will be updated at the end of it, and we don't need to trigger that update here.
473471
if (!resolvingAddresses) {
474-
if (newState == IDLE) {
475-
lb.requestConnection();
476-
}
477472
updateOverallBalancingState();
478473
}
479474
}

‎util/src/main/java/io/grpc/util/RoundRobinLoadBalancer.java‎

Lines changed: 19 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -95,6 +95,25 @@ private SubchannelPicker createReadyPicker(Collection<ChildLbState> children) {
9595
return new ReadyPicker(pickerList, sequence);
9696
}
9797

98+
@Override
99+
protected ChildLbState createChildLbState(Object key, Object policyConfig,
100+
SubchannelPicker initialPicker, ResolvedAddresses resolvedAddresses) {
101+
return new ChildLbState(key, pickFirstLbProvider, policyConfig, initialPicker) {
102+
@Override
103+
protected ChildLbStateHelper createChildHelper() {
104+
return new ChildLbStateHelper() {
105+
@Override
106+
public void updateBalancingState(ConnectivityState newState, SubchannelPicker newPicker) {
107+
super.updateBalancingState(newState, newPicker);
108+
if (!resolvingAddresses && newState == IDLE) {
109+
getLb().requestConnection();
110+
}
111+
}
112+
};
113+
}
114+
};
115+
}
116+
98117
@VisibleForTesting
99118
static class ReadyPicker extends SubchannelPicker {
100119
private final List<SubchannelPicker> subchannelPickers; // non-empty

‎xds/src/main/java/io/grpc/xds/LeastRequestLoadBalancer.java‎

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -328,5 +328,18 @@ public LeastRequestLbState(Object key, LoadBalancerProvider policyProvider,
328328
int getActiveRequests() {
329329
return activeRequests.get();
330330
}
331+
332+
@Override
333+
protected ChildLbStateHelper createChildHelper() {
334+
return new ChildLbStateHelper() {
335+
@Override
336+
public void updateBalancingState(ConnectivityState newState, SubchannelPicker newPicker) {
337+
super.updateBalancingState(newState, newPicker);
338+
if (!resolvingAddresses && newState == IDLE) {
339+
getLb().requestConnection();
340+
}
341+
}
342+
};
343+
}
331344
}
332345
}

‎xds/src/main/java/io/grpc/xds/RingHashLoadBalancer.java‎

Lines changed: 6 additions & 43 deletions
Original file line numberDiff line numberDiff line change
@@ -229,7 +229,7 @@ protected void updateOverallBalancingState() {
229229
@Override
230230
protected ChildLbState createChildLbState(Object key, Object policyConfig,
231231
SubchannelPicker initialPicker, ResolvedAddresses resolvedAddresses) {
232-
return new RingHashChildLbState((Endpoint)key);
232+
return new ChildLbState(key, lazyLbFactory, null, EMPTY_PICKER);
233233
}
234234

235235
private Status validateAddrList(List<EquivalentAddressGroup> addrList) {
@@ -358,7 +358,7 @@ private RingHashPicker(
358358
this.ring = ring;
359359
pickableSubchannels = new HashMap<>(subchannels.size());
360360
for (Map.Entry<Object, ChildLbState> entry : subchannels.entrySet()) {
361-
RingHashChildLbState childLbState = (RingHashChildLbState) entry.getValue();
361+
ChildLbState childLbState = entry.getValue();
362362
pickableSubchannels.put((Endpoint)entry.getKey(),
363363
new SubchannelView(childLbState, childLbState.getCurrentState()));
364364
}
@@ -405,7 +405,7 @@ public PickResult pickSubchannel(PickSubchannelArgs args) {
405405
for (int i = 0; i < ring.size(); i++) {
406406
int index = (targetIndex + i) % ring.size();
407407
SubchannelView subchannelView = pickableSubchannels.get(ring.get(index).addrKey);
408-
RingHashChildLbState childLbState = subchannelView.childLbState;
408+
ChildLbState childLbState = subchannelView.childLbState;
409409

410410
if (subchannelView.connectivityState == READY) {
411411
return childLbState.getCurrentPicker().pickSubchannel(args);
@@ -427,7 +427,7 @@ public PickResult pickSubchannel(PickSubchannelArgs args) {
427427
}
428428

429429
// return the pick from the original subchannel hit by hash, which is probably an error
430-
RingHashChildLbState originalSubchannel =
430+
ChildLbState originalSubchannel =
431431
pickableSubchannels.get(ring.get(targetIndex).addrKey).childLbState;
432432
return originalSubchannel.getCurrentPicker().pickSubchannel(args);
433433
}
@@ -439,10 +439,10 @@ public PickResult pickSubchannel(PickSubchannelArgs args) {
439439
* state changes.
440440
*/
441441
private static final class SubchannelView {
442-
private final RingHashChildLbState childLbState;
442+
private final ChildLbState childLbState;
443443
private final ConnectivityState connectivityState;
444444

445-
private SubchannelView(RingHashChildLbState childLbState, ConnectivityState state) {
445+
private SubchannelView(ChildLbState childLbState, ConnectivityState state) {
446446
this.childLbState = childLbState;
447447
this.connectivityState = state;
448448
}
@@ -487,41 +487,4 @@ public String toString() {
487487
.toString();
488488
}
489489
}
490-
491-
class RingHashChildLbState extends MultiChildLoadBalancer.ChildLbState {
492-
493-
public RingHashChildLbState(Endpoint key) {
494-
super(key, lazyLbFactory, null, EMPTY_PICKER);
495-
}
496-
497-
@Override
498-
protected ChildLbStateHelper createChildHelper() {
499-
return new RingHashChildHelper();
500-
}
501-
502-
// Need to expose this to the LB class
503-
@Override
504-
protected void shutdown() {
505-
super.shutdown();
506-
}
507-
508-
private class RingHashChildHelper extends ChildLbStateHelper {
509-
@Override
510-
public void updateBalancingState(final ConnectivityState newState,
511-
final SubchannelPicker newPicker) {
512-
setCurrentState(newState);
513-
setCurrentPicker(newPicker);
514-
515-
if (getChildLbState(getKey()) == null) {
516-
return;
517-
}
518-
519-
// If we are already in the process of resolving addresses, the overall balancing state
520-
// will be updated at the end of it, and we don't need to trigger that update here.
521-
if (!resolvingAddresses) {
522-
updateOverallBalancingState();
523-
}
524-
}
525-
}
526-
}
527490
}

‎xds/src/main/java/io/grpc/xds/WeightedRoundRobinLoadBalancer.java‎

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -340,6 +340,14 @@ final class WrrChildLbStateHelper extends ChildLbStateHelper {
340340
public Subchannel createSubchannel(CreateSubchannelArgs args) {
341341
return new WrrSubchannel(super.createSubchannel(args), WeightedChildLbState.this);
342342
}
343+
344+
@Override
345+
public void updateBalancingState(ConnectivityState newState, SubchannelPicker newPicker) {
346+
super.updateBalancingState(newState, newPicker);
347+
if (!resolvingAddresses && newState == ConnectivityState.IDLE) {
348+
getLb().requestConnection();
349+
}
350+
}
343351
}
344352

345353
final class OrcaReportListener implements OrcaPerRequestReportListener, OrcaOobReportListener {

‎xds/src/test/java/io/grpc/xds/RingHashLoadBalancerTest.java‎

Lines changed: 2 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -66,7 +66,6 @@
6666
import io.grpc.testing.TestMethodDescriptors;
6767
import io.grpc.util.AbstractTestHelper;
6868
import io.grpc.util.MultiChildLoadBalancer.ChildLbState;
69-
import io.grpc.xds.RingHashLoadBalancer.RingHashChildLbState;
7069
import io.grpc.xds.RingHashLoadBalancer.RingHashConfig;
7170
import java.lang.Thread.UncaughtExceptionHandler;
7271
import java.net.SocketAddress;
@@ -177,8 +176,7 @@ public void subchannelNotAutoReconnectAfterReenteringIdle() {
177176
assertThat(addressesAcceptanceStatus.isOk()).isTrue();
178177
verify(helper).updateBalancingState(eq(IDLE), pickerCaptor.capture());
179178

180-
RingHashChildLbState childLbState =
181-
(RingHashChildLbState) loadBalancer.getChildLbStates().iterator().next();
179+
ChildLbState childLbState = loadBalancer.getChildLbStates().iterator().next();
182180
assertThat(subchannels.get(Collections.singletonList(childLbState.getEag()))).isNull();
183181

184182
// Picking subchannel triggers connection.
@@ -422,7 +420,7 @@ public void skipFailingHosts_pickNextNonFailingHost() {
422420
assertThat(addressesAcceptanceStatus.isOk()).isTrue();
423421

424422
// Create subchannel for the first address
425-
((RingHashChildLbState) loadBalancer.getChildLbStateEag(servers.get(0))).getCurrentPicker()
423+
loadBalancer.getChildLbStateEag(servers.get(0)).getCurrentPicker()
426424
.pickSubchannel(getDefaultPickSubchannelArgs(hashFunc.hashVoid()));
427425
verifyConnection(1);
428426

0 commit comments

Comments
 (0)