Skip to content

Commit faf0add

Browse files
committed
Fix RLS test
lb_serverStatusCodeConversion() has been misleading since 42e1829 ("xds: Do RLS fallback policy eagar start"). At that point, the subchannel it marked as READY was for the default target's policy, not the policy for wilderness. However, since old PF policy provided a subchannel when CONNECTING, everything was "fine", but RLS would mistakenly count toward target_picks. This demonstrates that target_picks has been broken since it was introduced for PF, as PF relied on the caller to avoid the picker when it was CONNECTING. This may have been hard to notice in production, as the metrics become correct as soon as the connection is established, so as long as you use the channel for a while, the duplicate counting would become a small percentage of the overall amount.
1 parent 9287f85 commit faf0add

File tree

1 file changed

+7
-9
lines changed

1 file changed

+7
-9
lines changed

rls/src/test/java/io/grpc/rls/RlsLoadBalancerTest.java

Lines changed: 7 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -72,7 +72,6 @@
7272
import io.grpc.inprocess.InProcessServerBuilder;
7373
import io.grpc.internal.FakeClock;
7474
import io.grpc.internal.JsonParser;
75-
import io.grpc.internal.PickFirstLoadBalancerProvider;
7675
import io.grpc.internal.PickSubchannelArgsImpl;
7776
import io.grpc.internal.testing.StreamRecorder;
7877
import io.grpc.lookup.v1.RouteLookupServiceGrpc;
@@ -212,12 +211,14 @@ public void lb_serverStatusCodeConversion() throws Exception {
212211
throw new RuntimeException(e);
213212
}
214213
});
214+
assertThat(subchannels.poll()).isNotNull(); // default target
215+
assertThat(subchannels.poll()).isNull();
216+
// Warm-up pick; will be queued
215217
InOrder inOrder = inOrder(helper);
216218
inOrder.verify(helper)
217219
.updateBalancingState(eq(ConnectivityState.CONNECTING), pickerCaptor.capture());
218220
SubchannelPicker picker = pickerCaptor.getValue();
219221
PickSubchannelArgs fakeSearchMethodArgs = newPickSubchannelArgs(fakeSearchMethod);
220-
// Warm-up pick; will be queued
221222
PickResult res = picker.pickSubchannel(fakeSearchMethodArgs);
222223
assertThat(res.getStatus().isOk()).isTrue();
223224
assertThat(res.getSubchannel()).isNull();
@@ -230,8 +231,7 @@ public void lb_serverStatusCodeConversion() throws Exception {
230231
subchannel.updateState(ConnectivityStateInfo.forNonError(ConnectivityState.READY));
231232
res = picker.pickSubchannel(fakeSearchMethodArgs);
232233
assertThat(res.getStatus().getCode()).isEqualTo(Status.Code.OK);
233-
int expectedTimes = PickFirstLoadBalancerProvider.isEnabledNewPickFirst() ? 1 : 2;
234-
verifyLongCounterAdd("grpc.lb.rls.target_picks", expectedTimes, 1, "wilderness", "complete");
234+
verifyLongCounterAdd("grpc.lb.rls.target_picks", 1, 1, "wilderness", "complete");
235235

236236
// Check on conversion
237237
Throwable cause = new Throwable("cause");
@@ -284,8 +284,7 @@ public void lb_working_withDefaultTarget_rlsResponding() throws Exception {
284284
res = picker.pickSubchannel(searchSubchannelArgs);
285285
assertThat(subchannelIsReady(res.getSubchannel())).isTrue();
286286
assertThat(res.getSubchannel()).isSameInstanceAs(searchSubchannel);
287-
int expectedTimes = PickFirstLoadBalancerProvider.isEnabledNewPickFirst() ? 1 : 2;
288-
verifyLongCounterAdd("grpc.lb.rls.target_picks", expectedTimes, 1, "wilderness", "complete");
287+
verifyLongCounterAdd("grpc.lb.rls.target_picks", 1, 1, "wilderness", "complete");
289288

290289
// rescue should be pending status although the overall channel state is READY
291290
res = picker.pickSubchannel(rescueSubchannelArgs);
@@ -431,7 +430,7 @@ public void lb_working_withDefaultTarget_noRlsResponse() throws Exception {
431430
inOrder.verify(helper).getMetricRecorder();
432431
inOrder.verify(helper).getChannelTarget();
433432
inOrder.verifyNoMoreInteractions();
434-
int times = PickFirstLoadBalancerProvider.isEnabledNewPickFirst() ? 1 : 2;
433+
int times = 1;
435434
verifyLongCounterAdd("grpc.lb.rls.default_target_picks", times, 1,
436435
"defaultTarget", "complete");
437436

@@ -536,8 +535,7 @@ public void lb_working_withoutDefaultTarget() throws Exception {
536535
res = picker.pickSubchannel(newPickSubchannelArgs(fakeSearchMethod));
537536
assertThat(res.getStatus().isOk()).isFalse();
538537
assertThat(subchannelIsReady(res.getSubchannel())).isFalse();
539-
int expectedTimes = PickFirstLoadBalancerProvider.isEnabledNewPickFirst() ? 1 : 2;
540-
verifyLongCounterAdd("grpc.lb.rls.target_picks", expectedTimes, 1, "wilderness", "complete");
538+
verifyLongCounterAdd("grpc.lb.rls.target_picks", 1, 1, "wilderness", "complete");
541539

542540
res = picker.pickSubchannel(newPickSubchannelArgs(fakeRescueMethod));
543541
assertThat(subchannelIsReady(res.getSubchannel())).isTrue();

0 commit comments

Comments
 (0)