Skip to content

Commit 59b189b

Browse files
larry-safranlarry-safran
authored
Change HappyEyeballs and new pick first LB flags default value to false (grpc#11120)
* Change HappyEyeballs flag default value to false since some G3 users are seeing problems. Put the flag logic in a common place for PickFirstLeafLoadBalancer & WRR's test. * Set expected requestConnection count based on whether happy eyeballs is enabled or not * Disable new PickFirstLB * Fix test expectations to handle both new and old PF LB paths.
1 parent d366d74 commit 59b189b

File tree

7 files changed

+47
-21
lines changed

7 files changed

+47
-21
lines changed

‎core/src/main/java/io/grpc/internal/PickFirstLeafLoadBalancer.java‎

Lines changed: 1 addition & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -59,8 +59,6 @@ final class PickFirstLeafLoadBalancer extends LoadBalancer {
5959
private static final Logger log = Logger.getLogger(PickFirstLeafLoadBalancer.class.getName());
6060
@VisibleForTesting
6161
static final int CONNECTION_DELAY_INTERVAL_MS = 250;
62-
public static final String GRPC_EXPERIMENTAL_XDS_DUALSTACK_ENDPOINTS =
63-
"GRPC_EXPERIMENTAL_XDS_DUALSTACK_ENDPOINTS";
6462
private final Helper helper;
6563
private final Map<SocketAddress, SubchannelData> subchannels = new HashMap<>();
6664
private Index addressIndex;
@@ -71,7 +69,7 @@ final class PickFirstLeafLoadBalancer extends LoadBalancer {
7169
private ConnectivityState rawConnectivityState = IDLE;
7270
private ConnectivityState concludedState = IDLE;
7371
private final boolean enableHappyEyeballs =
74-
GrpcUtil.getFlag(GRPC_EXPERIMENTAL_XDS_DUALSTACK_ENDPOINTS, true);
72+
PickFirstLoadBalancerProvider.isEnabledHappyEyeballs();
7573

7674
PickFirstLeafLoadBalancer(Helper helper) {
7775
this.helper = checkNotNull(helper, "helper");

‎core/src/main/java/io/grpc/internal/PickFirstLoadBalancerProvider.java‎

Lines changed: 12 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -33,10 +33,21 @@
3333
* down the address list and sticks to the first that works.
3434
*/
3535
public final class PickFirstLoadBalancerProvider extends LoadBalancerProvider {
36+
public static final String GRPC_EXPERIMENTAL_XDS_DUALSTACK_ENDPOINTS =
37+
"GRPC_EXPERIMENTAL_XDS_DUALSTACK_ENDPOINTS";
3638
private static final String SHUFFLE_ADDRESS_LIST_KEY = "shuffleAddressList";
3739

3840
static boolean enableNewPickFirst =
39-
GrpcUtil.getFlag("GRPC_EXPERIMENTAL_ENABLE_NEW_PICK_FIRST", true);
41+
GrpcUtil.getFlag("GRPC_EXPERIMENTAL_ENABLE_NEW_PICK_FIRST", false);
42+
43+
public static boolean isEnabledHappyEyeballs() {
44+
return GrpcUtil.getFlag(GRPC_EXPERIMENTAL_XDS_DUALSTACK_ENDPOINTS, false);
45+
}
46+
47+
@VisibleForTesting
48+
public static boolean isEnableNewPickFirst() {
49+
return enableNewPickFirst;
50+
}
4051

4152
@Override
4253
public boolean isAvailable() {

‎core/src/test/java/io/grpc/internal/PickFirstLeafLoadBalancerTest.java‎

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -142,8 +142,8 @@ public void uncaughtException(Thread t, Throwable e) {
142142
@Before
143143
public void setUp() {
144144
originalHappyEyeballsEnabledValue =
145-
System.getProperty(PickFirstLeafLoadBalancer.GRPC_EXPERIMENTAL_XDS_DUALSTACK_ENDPOINTS);
146-
System.setProperty(PickFirstLeafLoadBalancer.GRPC_EXPERIMENTAL_XDS_DUALSTACK_ENDPOINTS,
145+
System.getProperty(PickFirstLoadBalancerProvider.GRPC_EXPERIMENTAL_XDS_DUALSTACK_ENDPOINTS);
146+
System.setProperty(PickFirstLoadBalancerProvider.GRPC_EXPERIMENTAL_XDS_DUALSTACK_ENDPOINTS,
147147
enableHappyEyeballs ? "true" : "false");
148148

149149
for (int i = 1; i <= 5; i++) {
@@ -173,9 +173,9 @@ public void setUp() {
173173
@After
174174
public void tearDown() {
175175
if (originalHappyEyeballsEnabledValue == null) {
176-
System.clearProperty(PickFirstLeafLoadBalancer.GRPC_EXPERIMENTAL_XDS_DUALSTACK_ENDPOINTS);
176+
System.clearProperty(PickFirstLoadBalancerProvider.GRPC_EXPERIMENTAL_XDS_DUALSTACK_ENDPOINTS);
177177
} else {
178-
System.setProperty(PickFirstLeafLoadBalancer.GRPC_EXPERIMENTAL_XDS_DUALSTACK_ENDPOINTS,
178+
System.setProperty(PickFirstLoadBalancerProvider.GRPC_EXPERIMENTAL_XDS_DUALSTACK_ENDPOINTS,
179179
originalHappyEyeballsEnabledValue);
180180
}
181181

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

Lines changed: 20 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -70,6 +70,7 @@
7070
import io.grpc.inprocess.InProcessServerBuilder;
7171
import io.grpc.internal.FakeClock;
7272
import io.grpc.internal.JsonParser;
73+
import io.grpc.internal.PickFirstLoadBalancerProvider;
7374
import io.grpc.internal.PickSubchannelArgsImpl;
7475
import io.grpc.internal.testing.StreamRecorder;
7576
import io.grpc.lookup.v1.RouteLookupServiceGrpc;
@@ -212,7 +213,8 @@ public void lb_serverStatusCodeConversion() throws Exception {
212213
subchannel.updateState(ConnectivityStateInfo.forNonError(ConnectivityState.READY));
213214
res = picker.pickSubchannel(fakeSearchMethodArgs);
214215
assertThat(res.getStatus().getCode()).isEqualTo(Status.Code.OK);
215-
verifyLongCounterAdd("grpc.lb.rls.target_picks", 1, 1, "wilderness", "complete");
216+
int expectedTimes = PickFirstLoadBalancerProvider.isEnabledNewPickFirst() ? 1 : 2;
217+
verifyLongCounterAdd("grpc.lb.rls.target_picks", expectedTimes, 1, "wilderness", "complete");
216218

217219
// Check on conversion
218220
Throwable cause = new Throwable("cause");
@@ -244,6 +246,8 @@ public void lb_working_withDefaultTarget_rlsResponding() throws Exception {
244246
.updateBalancingState(eq(ConnectivityState.CONNECTING), any(SubchannelPicker.class));
245247
inOrder.verify(helper, atLeast(0)).getSynchronizationContext();
246248
inOrder.verify(helper, atLeast(0)).getScheduledExecutorService();
249+
inOrder.verify(helper, atLeast(0)).getMetricRecorder();
250+
inOrder.verify(helper, atLeast(0)).getChannelTarget();
247251
inOrder.verifyNoMoreInteractions();
248252

249253
assertThat(res.getStatus().isOk()).isTrue();
@@ -258,7 +262,8 @@ public void lb_working_withDefaultTarget_rlsResponding() throws Exception {
258262
res = picker.pickSubchannel(searchSubchannelArgs);
259263
assertThat(subchannelIsReady(res.getSubchannel())).isTrue();
260264
assertThat(res.getSubchannel()).isSameInstanceAs(searchSubchannel);
261-
verifyLongCounterAdd("grpc.lb.rls.target_picks", 1, 1, "wilderness", "complete");
265+
int expectedTimes = PickFirstLoadBalancerProvider.isEnabledNewPickFirst() ? 1 : 2;
266+
verifyLongCounterAdd("grpc.lb.rls.target_picks", expectedTimes, 1, "wilderness", "complete");
262267

263268
// rescue should be pending status although the overall channel state is READY
264269
res = picker.pickSubchannel(rescueSubchannelArgs);
@@ -367,18 +372,22 @@ public void lb_working_withDefaultTarget_noRlsResponse() throws Exception {
367372
inOrder.verify(helper).getMetricRecorder();
368373
inOrder.verify(helper).getChannelTarget();
369374
inOrder.verifyNoMoreInteractions();
370-
verifyLongCounterAdd("grpc.lb.rls.default_target_picks", 1, 1, "defaultTarget", "complete");
375+
int times = PickFirstLoadBalancerProvider.isEnabledNewPickFirst() ? 1 : 2;
376+
verifyLongCounterAdd("grpc.lb.rls.default_target_picks", times, 1,
377+
"defaultTarget", "complete");
371378
verifyNoMoreInteractions(mockMetricRecorder);
372379

373380
Subchannel subchannel = picker.pickSubchannel(searchSubchannelArgs).getSubchannel();
374381
assertThat(subchannelIsReady(subchannel)).isTrue();
375382
assertThat(subchannel).isSameInstanceAs(fallbackSubchannel);
376-
verifyLongCounterAdd("grpc.lb.rls.default_target_picks", 2, 1, "defaultTarget", "complete");
383+
verifyLongCounterAdd("grpc.lb.rls.default_target_picks", ++times, 1, "defaultTarget",
384+
"complete");
377385

378386
subchannel = picker.pickSubchannel(searchSubchannelArgs).getSubchannel();
379387
assertThat(subchannelIsReady(subchannel)).isTrue();
380388
assertThat(subchannel).isSameInstanceAs(fallbackSubchannel);
381-
verifyLongCounterAdd("grpc.lb.rls.default_target_picks", 3, 1, "defaultTarget", "complete");
389+
verifyLongCounterAdd("grpc.lb.rls.default_target_picks", ++times, 1, "defaultTarget",
390+
"complete");
382391

383392
// Make sure that when RLS starts communicating that default stops being used
384393
fakeThrottler.nextResult = false;
@@ -389,7 +398,8 @@ public void lb_working_withDefaultTarget_noRlsResponse() throws Exception {
389398
(FakeSubchannel) markReadyAndGetPickResult(inOrder, searchSubchannelArgs).getSubchannel();
390399
assertThat(searchSubchannel).isNotNull();
391400
assertThat(searchSubchannel).isNotSameInstanceAs(fallbackSubchannel);
392-
verifyLongCounterAdd("grpc.lb.rls.target_picks", 1, 1, "wilderness", "complete");
401+
times = PickFirstLoadBalancerProvider.isEnabledNewPickFirst() ? 1 : 2;
402+
verifyLongCounterAdd("grpc.lb.rls.target_picks", times, 1, "wilderness", "complete");
393403

394404
// create rescue subchannel
395405
picker.pickSubchannel(rescueSubchannelArgs);
@@ -433,6 +443,8 @@ public void lb_working_withoutDefaultTarget() throws Exception {
433443
.updateBalancingState(eq(ConnectivityState.CONNECTING), any(SubchannelPicker.class));
434444
inOrder.verify(helper, atLeast(0)).getSynchronizationContext();
435445
inOrder.verify(helper, atLeast(0)).getScheduledExecutorService();
446+
inOrder.verify(helper, atLeast(0)).getMetricRecorder();
447+
inOrder.verify(helper, atLeast(0)).getChannelTarget();
436448
inOrder.verifyNoMoreInteractions();
437449
assertThat(res.getStatus().isOk()).isTrue();
438450

@@ -469,7 +481,8 @@ public void lb_working_withoutDefaultTarget() throws Exception {
469481
res = picker.pickSubchannel(newPickSubchannelArgs(fakeSearchMethod));
470482
assertThat(res.getStatus().isOk()).isFalse();
471483
assertThat(subchannelIsReady(res.getSubchannel())).isFalse();
472-
verifyLongCounterAdd("grpc.lb.rls.target_picks", 1, 1, "wilderness", "complete");
484+
int expectedTimes = PickFirstLoadBalancerProvider.isEnabledNewPickFirst() ? 1 : 2;
485+
verifyLongCounterAdd("grpc.lb.rls.target_picks", expectedTimes, 1, "wilderness", "complete");
473486

474487
res = picker.pickSubchannel(newPickSubchannelArgs(fakeRescueMethod));
475488
assertThat(subchannelIsReady(res.getSubchannel())).isTrue();

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

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -101,7 +101,7 @@ protected EdsUpdate doParse(Args args, Message unpackedMessage) throws ResourceI
101101
}
102102

103103
private static boolean isEnabledXdsDualStack() {
104-
return GrpcUtil.getFlag(GRPC_EXPERIMENTAL_XDS_DUALSTACK_ENDPOINTS, true);
104+
return GrpcUtil.getFlag(GRPC_EXPERIMENTAL_XDS_DUALSTACK_ENDPOINTS, false);
105105
}
106106

107107
private static EdsUpdate processClusterLoadAssignment(ClusterLoadAssignment assignment)

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

Lines changed: 7 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -151,7 +151,8 @@ public void subchannelLazyConnectUntilPicked() {
151151
assertThat(result.getStatus().isOk()).isTrue();
152152
assertThat(result.getSubchannel()).isNull();
153153
Subchannel subchannel = Iterables.getOnlyElement(subchannels.values());
154-
verify(subchannel).requestConnection();
154+
int expectedTimes = PickFirstLoadBalancerProvider.isEnabledHappyEyeballs() ? 1 : 2;
155+
verify(subchannel, times(expectedTimes)).requestConnection();
155156
verify(helper).updateBalancingState(eq(CONNECTING), any(SubchannelPicker.class));
156157
verify(helper).createSubchannel(any(CreateSubchannelArgs.class));
157158
deliverSubchannelState(subchannel, CSI_CONNECTING);
@@ -185,7 +186,8 @@ public void subchannelNotAutoReconnectAfterReenteringIdle() {
185186
pickerCaptor.getValue().pickSubchannel(args);
186187
Subchannel subchannel = subchannels.get(Collections.singletonList(childLbState.getEag()));
187188
InOrder inOrder = Mockito.inOrder(helper, subchannel);
188-
inOrder.verify(subchannel).requestConnection();
189+
int expectedTimes = PickFirstLoadBalancerProvider.isEnabledHappyEyeballs() ? 1 : 2;
190+
inOrder.verify(subchannel, times(expectedTimes)).requestConnection();
189191
deliverSubchannelState(subchannel, CSI_READY);
190192
inOrder.verify(helper).updateBalancingState(eq(READY), any(SubchannelPicker.class));
191193
deliverSubchannelState(subchannel, ConnectivityStateInfo.forNonError(IDLE));
@@ -443,7 +445,9 @@ public void skipFailingHosts_pickNextNonFailingHost() {
443445
PickResult result = pickerCaptor.getValue().pickSubchannel(args);
444446
assertThat(result.getStatus().isOk()).isTrue();
445447
assertThat(result.getSubchannel()).isNull(); // buffer request
446-
verify(getSubChannel(servers.get(1))).requestConnection(); // kicked off connection to server2
448+
int expectedTimes = PickFirstLoadBalancerProvider.isEnabledHappyEyeballs() ? 1 : 2;
449+
// verify kicked off connection to server2
450+
verify(getSubChannel(servers.get(1)), times(expectedTimes)).requestConnection();
447451
assertThat(subchannels.size()).isEqualTo(2); // no excessive connection
448452

449453
deliverSubchannelState(getSubChannel(servers.get(1)), CSI_CONNECTING);

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

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -64,7 +64,7 @@
6464
import io.grpc.inprocess.InProcessChannelBuilder;
6565
import io.grpc.inprocess.InProcessServerBuilder;
6666
import io.grpc.internal.FakeClock;
67-
import io.grpc.internal.GrpcUtil;
67+
import io.grpc.internal.PickFirstLoadBalancerProvider;
6868
import io.grpc.internal.TestUtils;
6969
import io.grpc.internal.testing.StreamRecorder;
7070
import io.grpc.services.InternalCallMetricRecorder;
@@ -595,7 +595,7 @@ weightedChild2.new OrcaReportListener(weightedConfig.errorUtilizationPenalty).on
595595
}
596596

597597
private boolean isEnabledHappyEyeballs() {
598-
return GrpcUtil.getFlag("GRPC_EXPERIMENTAL_XDS_DUALSTACK_ENDPOINTS", true);
598+
return PickFirstLoadBalancerProvider.isEnabledHappyEyeballs();
599599
}
600600

601601
@Test

0 commit comments

Comments
 (0)