Skip to content

Commit 57124d6

Browse files
ejona86ejona86
committed
Use acceptResolvedAddresses() in easy cases
We want to move away from handleResolvedAddresses(). These are "easy" in that they need no logic. LBs extending ForwardingLoadBalancer had the method duplicated from handleResolvedAddresses() and swapped away from `super` because ForwardingLoadBalancer only forwards handleResolvedAddresses() reliably today. Duplicating small methods was less bug-prone than dealing with ForwardingLoadBalancer.
1 parent 110c1ff commit 57124d6

File tree

11 files changed

+95
-62
lines changed

11 files changed

+95
-62
lines changed

‎interop-testing/src/main/java/io/grpc/testing/integration/RpcBehaviorLoadBalancerProvider.java

+7
Original file line numberDiff line numberDiff line change
@@ -116,6 +116,13 @@ public void handleResolvedAddresses(ResolvedAddresses resolvedAddresses) {
116116
((RpcBehaviorConfig) resolvedAddresses.getLoadBalancingPolicyConfig()).rpcBehavior);
117117
delegateLb.handleResolvedAddresses(resolvedAddresses);
118118
}
119+
120+
@Override
121+
public Status acceptResolvedAddresses(ResolvedAddresses resolvedAddresses) {
122+
helper.setRpcBehavior(
123+
((RpcBehaviorConfig) resolvedAddresses.getLoadBalancingPolicyConfig()).rpcBehavior);
124+
return delegateLb.acceptResolvedAddresses(resolvedAddresses);
125+
}
119126
}
120127

121128
/**

‎interop-testing/src/test/java/io/grpc/testing/integration/RpcBehaviorLoadBalancerProviderTest.java

+9
Original file line numberDiff line numberDiff line change
@@ -87,6 +87,15 @@ public void handleResolvedAddressesDelegated() {
8787
verify(mockDelegateLb).handleResolvedAddresses(resolvedAddresses);
8888
}
8989

90+
@Test
91+
public void acceptResolvedAddressesDelegated() {
92+
RpcBehaviorLoadBalancer lb = new RpcBehaviorLoadBalancer(new RpcBehaviorHelper(mockHelper),
93+
mockDelegateLb);
94+
ResolvedAddresses resolvedAddresses = buildResolvedAddresses(buildConfig());
95+
lb.acceptResolvedAddresses(resolvedAddresses);
96+
verify(mockDelegateLb).acceptResolvedAddresses(resolvedAddresses);
97+
}
98+
9099
@Test
91100
public void helperWrapsPicker() {
92101
RpcBehaviorHelper helper = new RpcBehaviorHelper(mockHelper);

‎services/src/main/java/io/grpc/protobuf/services/HealthCheckingLoadBalancerFactory.java

+12-1
Original file line numberDiff line numberDiff line change
@@ -194,7 +194,18 @@ public void handleResolvedAddresses(ResolvedAddresses resolvedAddresses) {
194194
.get(LoadBalancer.ATTR_HEALTH_CHECKING_CONFIG);
195195
String serviceName = ServiceConfigUtil.getHealthCheckedServiceName(healthCheckingConfig);
196196
helper.setHealthCheckedService(serviceName);
197-
super.handleResolvedAddresses(resolvedAddresses);
197+
delegate.handleResolvedAddresses(resolvedAddresses);
198+
}
199+
200+
@Override
201+
public Status acceptResolvedAddresses(ResolvedAddresses resolvedAddresses) {
202+
Map<String, ?> healthCheckingConfig =
203+
resolvedAddresses
204+
.getAttributes()
205+
.get(LoadBalancer.ATTR_HEALTH_CHECKING_CONFIG);
206+
String serviceName = ServiceConfigUtil.getHealthCheckedServiceName(healthCheckingConfig);
207+
helper.setHealthCheckedService(serviceName);
208+
return delegate.acceptResolvedAddresses(resolvedAddresses);
198209
}
199210

200211
@Override

‎services/src/test/java/io/grpc/protobuf/services/HealthCheckingLoadBalancerFactoryTest.java

+49-48
Original file line numberDiff line numberDiff line change
@@ -206,15 +206,16 @@ public void setup() throws Exception {
206206
boolean shutdown;
207207

208208
@Override
209-
public void handleResolvedAddresses(final ResolvedAddresses resolvedAddresses) {
209+
public Status acceptResolvedAddresses(final ResolvedAddresses resolvedAddresses) {
210210
syncContext.execute(new Runnable() {
211211
@Override
212212
public void run() {
213213
if (!shutdown) {
214-
hcLb.handleResolvedAddresses(resolvedAddresses);
214+
hcLb.acceptResolvedAddresses(resolvedAddresses);
215215
}
216216
}
217217
});
218+
return Status.OK;
218219
}
219220

220221
@Override
@@ -264,9 +265,9 @@ public void typicalWorkflow() {
264265
.setAddresses(resolvedAddressList)
265266
.setAttributes(resolutionAttrs)
266267
.build();
267-
hcLbEventDelivery.handleResolvedAddresses(result);
268+
hcLbEventDelivery.acceptResolvedAddresses(result);
268269

269-
verify(origLb).handleResolvedAddresses(result);
270+
verify(origLb).acceptResolvedAddresses(result);
270271
verify(origHelper, atLeast(0)).getSynchronizationContext();
271272
verify(origHelper, atLeast(0)).getScheduledExecutorService();
272273
verifyNoMoreInteractions(origHelper);
@@ -404,9 +405,9 @@ public void healthCheckDisabledWhenServiceNotImplemented() {
404405
.setAddresses(resolvedAddressList)
405406
.setAttributes(resolutionAttrs)
406407
.build();
407-
hcLbEventDelivery.handleResolvedAddresses(result);
408+
hcLbEventDelivery.acceptResolvedAddresses(result);
408409

409-
verify(origLb).handleResolvedAddresses(result);
410+
verify(origLb).acceptResolvedAddresses(result);
410411
verifyNoMoreInteractions(origLb);
411412

412413
// We create 2 Subchannels. One of them connects to a server that doesn't implement health check
@@ -489,9 +490,9 @@ public void backoffRetriesWhenServerErroneouslyClosesRpcBeforeAnyResponse() {
489490
.setAddresses(resolvedAddressList)
490491
.setAttributes(resolutionAttrs)
491492
.build();
492-
hcLbEventDelivery.handleResolvedAddresses(result);
493+
hcLbEventDelivery.acceptResolvedAddresses(result);
493494

494-
verify(origLb).handleResolvedAddresses(result);
495+
verify(origLb).acceptResolvedAddresses(result);
495496
verifyNoMoreInteractions(origLb);
496497

497498
SubchannelStateListener mockHealthListener = mockHealthListeners[0];
@@ -567,9 +568,9 @@ public void serverRespondResetsBackoff() {
567568
.setAddresses(resolvedAddressList)
568569
.setAttributes(resolutionAttrs)
569570
.build();
570-
hcLbEventDelivery.handleResolvedAddresses(result);
571+
hcLbEventDelivery.acceptResolvedAddresses(result);
571572

572-
verify(origLb).handleResolvedAddresses(result);
573+
verify(origLb).acceptResolvedAddresses(result);
573574
verifyNoMoreInteractions(origLb);
574575

575576
SubchannelStateListener mockStateListener = mockStateListeners[0];
@@ -667,9 +668,9 @@ public void serviceConfigHasNoHealthCheckingInitiallyButDoesLater() {
667668
.setAddresses(resolvedAddressList)
668669
.setAttributes(Attributes.EMPTY)
669670
.build();
670-
hcLbEventDelivery.handleResolvedAddresses(result1);
671+
hcLbEventDelivery.acceptResolvedAddresses(result1);
671672

672-
verify(origLb).handleResolvedAddresses(result1);
673+
verify(origLb).acceptResolvedAddresses(result1);
673674
verifyNoMoreInteractions(origLb);
674675

675676
// First, create Subchannels 0
@@ -688,8 +689,8 @@ public void serviceConfigHasNoHealthCheckingInitiallyButDoesLater() {
688689
.setAddresses(resolvedAddressList)
689690
.setAttributes(resolutionAttrs)
690691
.build();
691-
hcLbEventDelivery.handleResolvedAddresses(result2);
692-
verify(origLb).handleResolvedAddresses(result2);
692+
hcLbEventDelivery.acceptResolvedAddresses(result2);
693+
verify(origLb).acceptResolvedAddresses(result2);
693694

694695
// Health check started on existing Subchannel
695696
assertThat(healthImpls[0].calls).hasSize(1);
@@ -711,9 +712,9 @@ public void serviceConfigDisablesHealthCheckWhenRpcActive() {
711712
.setAddresses(resolvedAddressList)
712713
.setAttributes(resolutionAttrs)
713714
.build();
714-
hcLbEventDelivery.handleResolvedAddresses(result1);
715+
hcLbEventDelivery.acceptResolvedAddresses(result1);
715716

716-
verify(origLb).handleResolvedAddresses(result1);
717+
verify(origLb).acceptResolvedAddresses(result1);
717718
verifyNoMoreInteractions(origLb);
718719

719720
Subchannel subchannel = createSubchannel(0, Attributes.EMPTY, maybeGetMockListener());
@@ -738,15 +739,15 @@ public void serviceConfigDisablesHealthCheckWhenRpcActive() {
738739
.setAddresses(resolvedAddressList)
739740
.setAttributes(Attributes.EMPTY)
740741
.build();
741-
hcLbEventDelivery.handleResolvedAddresses(result2);
742+
hcLbEventDelivery.acceptResolvedAddresses(result2);
742743

743744
// Health check RPC cancelled.
744745
assertThat(serverCall.cancelled).isTrue();
745746
// Subchannel uses original state
746747
inOrder.verify(getMockListener()).onSubchannelState(
747748
eq(ConnectivityStateInfo.forNonError(READY)));
748749

749-
inOrder.verify(origLb).handleResolvedAddresses(result2);
750+
inOrder.verify(origLb).acceptResolvedAddresses(result2);
750751

751752
verifyNoMoreInteractions(origLb, mockStateListeners[0]);
752753
assertThat(healthImpl.calls).isEmpty();
@@ -759,9 +760,9 @@ public void serviceConfigDisablesHealthCheckWhenRetryPending() {
759760
.setAddresses(resolvedAddressList)
760761
.setAttributes(resolutionAttrs)
761762
.build();
762-
hcLbEventDelivery.handleResolvedAddresses(result);
763+
hcLbEventDelivery.acceptResolvedAddresses(result);
763764

764-
verify(origLb).handleResolvedAddresses(result);
765+
verify(origLb).acceptResolvedAddresses(result);
765766
verifyNoMoreInteractions(origLb);
766767

767768
SubchannelStateListener mockHealthListener = mockHealthListeners[0];
@@ -793,7 +794,7 @@ public void serviceConfigDisablesHealthCheckWhenRetryPending() {
793794
.setAddresses(resolvedAddressList)
794795
.setAttributes(Attributes.EMPTY)
795796
.build();
796-
hcLbEventDelivery.handleResolvedAddresses(result2);
797+
hcLbEventDelivery.acceptResolvedAddresses(result2);
797798

798799
// Retry timer is cancelled
799800
assertThat(clock.getPendingTasks()).isEmpty();
@@ -805,7 +806,7 @@ public void serviceConfigDisablesHealthCheckWhenRetryPending() {
805806
inOrder.verify(getMockListener()).onSubchannelState(
806807
eq(ConnectivityStateInfo.forNonError(READY)));
807808

808-
inOrder.verify(origLb).handleResolvedAddresses(result2);
809+
inOrder.verify(origLb).acceptResolvedAddresses(result2);
809810

810811
verifyNoMoreInteractions(origLb, mockStateListeners[0]);
811812
}
@@ -817,9 +818,9 @@ public void serviceConfigDisablesHealthCheckWhenRpcInactive() {
817818
.setAddresses(resolvedAddressList)
818819
.setAttributes(resolutionAttrs)
819820
.build();
820-
hcLbEventDelivery.handleResolvedAddresses(result1);
821+
hcLbEventDelivery.acceptResolvedAddresses(result1);
821822

822-
verify(origLb).handleResolvedAddresses(result1);
823+
verify(origLb).acceptResolvedAddresses(result1);
823824
verifyNoMoreInteractions(origLb);
824825

825826
Subchannel subchannel = createSubchannel(0, Attributes.EMPTY, maybeGetMockListener());
@@ -842,9 +843,9 @@ public void serviceConfigDisablesHealthCheckWhenRpcInactive() {
842843
.setAddresses(resolvedAddressList)
843844
.setAttributes(Attributes.EMPTY)
844845
.build();
845-
hcLbEventDelivery.handleResolvedAddresses(result2);
846+
hcLbEventDelivery.acceptResolvedAddresses(result2);
846847

847-
inOrder.verify(origLb).handleResolvedAddresses(result2);
848+
inOrder.verify(origLb).acceptResolvedAddresses(result2);
848849

849850
// Underlying subchannel is now ready
850851
deliverSubchannelState(0, ConnectivityStateInfo.forNonError(READY));
@@ -870,9 +871,9 @@ public void serviceConfigChangesServiceNameWhenRpcActive() {
870871
.setAddresses(resolvedAddressList)
871872
.setAttributes(resolutionAttrs)
872873
.build();
873-
hcLbEventDelivery.handleResolvedAddresses(result1);
874+
hcLbEventDelivery.acceptResolvedAddresses(result1);
874875

875-
verify(origLb).handleResolvedAddresses(result1);
876+
verify(origLb).acceptResolvedAddresses(result1);
876877
verifyNoMoreInteractions(origLb);
877878

878879
SubchannelStateListener mockHealthListener = mockHealthListeners[0];
@@ -900,9 +901,9 @@ public void serviceConfigChangesServiceNameWhenRpcActive() {
900901
eq(ConnectivityStateInfo.forNonError(READY)));
901902

902903
// Service config returns with the same health check name.
903-
hcLbEventDelivery.handleResolvedAddresses(result1);
904+
hcLbEventDelivery.acceptResolvedAddresses(result1);
904905
// It's delivered to origLb, but nothing else happens
905-
inOrder.verify(origLb).handleResolvedAddresses(result1);
906+
inOrder.verify(origLb).acceptResolvedAddresses(result1);
906907
verifyNoMoreInteractions(origLb, mockListener);
907908

908909
// Service config returns a different health check name.
@@ -911,8 +912,8 @@ public void serviceConfigChangesServiceNameWhenRpcActive() {
911912
.setAddresses(resolvedAddressList)
912913
.setAttributes(resolutionAttrs)
913914
.build();
914-
hcLbEventDelivery.handleResolvedAddresses(result2);
915-
inOrder.verify(origLb).handleResolvedAddresses(result2);
915+
hcLbEventDelivery.acceptResolvedAddresses(result2);
916+
inOrder.verify(origLb).acceptResolvedAddresses(result2);
916917

917918
// Current health check RPC cancelled.
918919
assertThat(serverCall.cancelled).isTrue();
@@ -934,9 +935,9 @@ public void serviceConfigChangesServiceNameWhenRetryPending() {
934935
.setAddresses(resolvedAddressList)
935936
.setAttributes(resolutionAttrs)
936937
.build();
937-
hcLbEventDelivery.handleResolvedAddresses(result1);
938+
hcLbEventDelivery.acceptResolvedAddresses(result1);
938939

939-
verify(origLb).handleResolvedAddresses(result1);
940+
verify(origLb).acceptResolvedAddresses(result1);
940941
verifyNoMoreInteractions(origLb);
941942

942943
SubchannelStateListener mockHealthListener = mockHealthListeners[0];
@@ -969,9 +970,9 @@ public void serviceConfigChangesServiceNameWhenRetryPending() {
969970

970971
// Service config returns with the same health check name.
971972

972-
hcLbEventDelivery.handleResolvedAddresses(result1);
973+
hcLbEventDelivery.acceptResolvedAddresses(result1);
973974
// It's delivered to origLb, but nothing else happens
974-
inOrder.verify(origLb).handleResolvedAddresses(result1);
975+
inOrder.verify(origLb).acceptResolvedAddresses(result1);
975976
verifyNoMoreInteractions(origLb, mockListener);
976977
assertThat(clock.getPendingTasks()).hasSize(1);
977978
assertThat(healthImpl.calls).isEmpty();
@@ -982,12 +983,12 @@ public void serviceConfigChangesServiceNameWhenRetryPending() {
982983
.setAddresses(resolvedAddressList)
983984
.setAttributes(resolutionAttrs)
984985
.build();
985-
hcLbEventDelivery.handleResolvedAddresses(result2);
986+
hcLbEventDelivery.acceptResolvedAddresses(result2);
986987
// Concluded CONNECTING state
987988
inOrder.verify(getMockListener()).onSubchannelState(
988989
eq(ConnectivityStateInfo.forNonError(CONNECTING)));
989990

990-
inOrder.verify(origLb).handleResolvedAddresses(result2);
991+
inOrder.verify(origLb).acceptResolvedAddresses(result2);
991992

992993
// Current retry timer cancelled
993994
assertThat(clock.getPendingTasks()).isEmpty();
@@ -1008,9 +1009,9 @@ public void serviceConfigChangesServiceNameWhenRpcInactive() {
10081009
.setAddresses(resolvedAddressList)
10091010
.setAttributes(resolutionAttrs)
10101011
.build();
1011-
hcLbEventDelivery.handleResolvedAddresses(result1);
1012+
hcLbEventDelivery.acceptResolvedAddresses(result1);
10121013

1013-
verify(origLb).handleResolvedAddresses(result1);
1014+
verify(origLb).acceptResolvedAddresses(result1);
10141015
verifyNoMoreInteractions(origLb);
10151016

10161017
Subchannel subchannel = createSubchannel(0, Attributes.EMPTY, maybeGetMockListener());
@@ -1031,9 +1032,9 @@ public void serviceConfigChangesServiceNameWhenRpcInactive() {
10311032
inOrder.verifyNoMoreInteractions();
10321033

10331034
// Service config returns with the same health check name.
1034-
hcLbEventDelivery.handleResolvedAddresses(result1);
1035+
hcLbEventDelivery.acceptResolvedAddresses(result1);
10351036
// It's delivered to origLb, but nothing else happens
1036-
inOrder.verify(origLb).handleResolvedAddresses(result1);
1037+
inOrder.verify(origLb).acceptResolvedAddresses(result1);
10371038
assertThat(healthImpl.calls).isEmpty();
10381039
verifyNoMoreInteractions(origLb);
10391040

@@ -1043,9 +1044,9 @@ public void serviceConfigChangesServiceNameWhenRpcInactive() {
10431044
.setAddresses(resolvedAddressList)
10441045
.setAttributes(resolutionAttrs)
10451046
.build();
1046-
hcLbEventDelivery.handleResolvedAddresses(result2);
1047+
hcLbEventDelivery.acceptResolvedAddresses(result2);
10471048

1048-
inOrder.verify(origLb).handleResolvedAddresses(result2);
1049+
inOrder.verify(origLb).acceptResolvedAddresses(result2);
10491050

10501051
// Underlying subchannel is now ready
10511052
deliverSubchannelState(0, ConnectivityStateInfo.forNonError(READY));
@@ -1092,9 +1093,9 @@ public void balancerShutdown() {
10921093
.setAddresses(resolvedAddressList)
10931094
.setAttributes(resolutionAttrs)
10941095
.build();
1095-
hcLbEventDelivery.handleResolvedAddresses(result);
1096+
hcLbEventDelivery.acceptResolvedAddresses(result);
10961097

1097-
verify(origLb).handleResolvedAddresses(result);
1098+
verify(origLb).acceptResolvedAddresses(result);
10981099
verifyNoMoreInteractions(origLb);
10991100
ServerSideCall[] serverCalls = new ServerSideCall[NUM_SUBCHANNELS];
11001101

@@ -1172,8 +1173,8 @@ public LoadBalancer newLoadBalancer(Helper helper) {
11721173
.setAddresses(resolvedAddressList)
11731174
.setAttributes(resolutionAttrs)
11741175
.build();
1175-
hcLbEventDelivery.handleResolvedAddresses(result);
1176-
verify(origLb).handleResolvedAddresses(result);
1176+
hcLbEventDelivery.acceptResolvedAddresses(result);
1177+
verify(origLb).acceptResolvedAddresses(result);
11771178
createSubchannel(0, Attributes.EMPTY);
11781179
assertThat(healthImpls[0].calls).isEmpty();
11791180
deliverSubchannelState(0, ConnectivityStateInfo.forNonError(READY));

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

+1-2
Original file line numberDiff line numberDiff line change
@@ -171,9 +171,8 @@ public Status acceptResolvedAddresses(ResolvedAddresses resolvedAddresses) {
171171
endpointTrackerMap.cancelTracking();
172172
}
173173

174-
switchLb.handleResolvedAddresses(
174+
return switchLb.acceptResolvedAddresses(
175175
resolvedAddresses.toBuilder().setLoadBalancingPolicyConfig(config.childConfig).build());
176-
return Status.OK;
177176
}
178177

179178
@Override

‎util/src/test/java/io/grpc/util/OutlierDetectionLoadBalancerTest.java

+1-1
Original file line numberDiff line numberDiff line change
@@ -280,7 +280,7 @@ public void acceptResolvedAddresses() {
280280
loadBalancer.acceptResolvedAddresses(resolvedAddresses);
281281

282282
// Handling of resolved addresses is delegated
283-
verify(mockChildLb).handleResolvedAddresses(
283+
verify(mockChildLb).acceptResolvedAddresses(
284284
resolvedAddresses.toBuilder().setLoadBalancingPolicyConfig(childConfig).build());
285285

286286
// There is a single pending task to run the outlier detection algorithm

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

+1-3
Original file line numberDiff line numberDiff line change
@@ -113,12 +113,10 @@ public Status acceptResolvedAddresses(ResolvedAddresses resolvedAddresses) {
113113
Object switchConfig = GracefulSwitchLoadBalancer.createLoadBalancingPolicyConfig(
114114
lbRegistry.getProvider(WEIGHTED_TARGET_POLICY_NAME),
115115
new WeightedTargetConfig(weightedPolicySelections));
116-
switchLb.handleResolvedAddresses(
116+
return switchLb.acceptResolvedAddresses(
117117
resolvedAddresses.toBuilder()
118118
.setLoadBalancingPolicyConfig(switchConfig)
119119
.build());
120-
121-
return Status.OK;
122120
}
123121

124122
@Override

‎xds/src/main/java/io/grpc/xds/orca/OrcaOobUtil.java

+1-1
Original file line numberDiff line numberDiff line change
@@ -83,7 +83,7 @@ private OrcaOobUtil() {}
8383
* class WrrLoadbalancer extends LoadBalancer {
8484
* private final Helper originHelper; // the original Helper
8585
*
86-
* public void handleResolvedAddresses(ResolvedAddresses resolvedAddresses) {
86+
* public Status acceptResolvedAddresses(ResolvedAddresses resolvedAddresses) {
8787
* // listener implements the logic for WRR's usage of backend metrics.
8888
* OrcaReportingHelper orcaHelper =
8989
* OrcaOobUtil.newOrcaReportingHelper(originHelper);

0 commit comments

Comments
 (0)