Skip to content

Commit f16b6f2

Browse files
larry-safranlarry-safran
authored
Change HappyEyeballs and new pick first LB flags default value to false (#11120) (#11177)
* 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 8f81bd2 commit f16b6f2

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
@@ -66,6 +66,7 @@
6666
import io.grpc.inprocess.InProcessServerBuilder;
6767
import io.grpc.internal.FakeClock;
6868
import io.grpc.internal.JsonParser;
69+
import io.grpc.internal.PickFirstLoadBalancerProvider;
6970
import io.grpc.internal.PickSubchannelArgsImpl;
7071
import io.grpc.lookup.v1.RouteLookupServiceGrpc;
7172
import io.grpc.rls.RlsLoadBalancer.CachingRlsLbClientBuilderProvider;
@@ -206,7 +207,8 @@ public void lb_serverStatusCodeConversion() throws Exception {
206207
subchannel.updateState(ConnectivityStateInfo.forNonError(ConnectivityState.READY));
207208
res = picker.pickSubchannel(fakeSearchMethodArgs);
208209
assertThat(res.getStatus().getCode()).isEqualTo(Status.Code.OK);
209-
verifyLongCounterAdd("grpc.lb.rls.target_picks", 1, 1, "wilderness", "complete");
210+
int expectedTimes = PickFirstLoadBalancerProvider.isEnabledNewPickFirst() ? 1 : 2;
211+
verifyLongCounterAdd("grpc.lb.rls.target_picks", expectedTimes, 1, "wilderness", "complete");
210212

211213
// Check on conversion
212214
Throwable cause = new Throwable("cause");
@@ -238,6 +240,8 @@ public void lb_working_withDefaultTarget_rlsResponding() throws Exception {
238240
.updateBalancingState(eq(ConnectivityState.CONNECTING), any(SubchannelPicker.class));
239241
inOrder.verify(helper, atLeast(0)).getSynchronizationContext();
240242
inOrder.verify(helper, atLeast(0)).getScheduledExecutorService();
243+
inOrder.verify(helper, atLeast(0)).getMetricRecorder();
244+
inOrder.verify(helper, atLeast(0)).getChannelTarget();
241245
inOrder.verifyNoMoreInteractions();
242246

243247
assertThat(res.getStatus().isOk()).isTrue();
@@ -252,7 +256,8 @@ public void lb_working_withDefaultTarget_rlsResponding() throws Exception {
252256
res = picker.pickSubchannel(searchSubchannelArgs);
253257
assertThat(subchannelIsReady(res.getSubchannel())).isTrue();
254258
assertThat(res.getSubchannel()).isSameInstanceAs(searchSubchannel);
255-
verifyLongCounterAdd("grpc.lb.rls.target_picks", 1, 1, "wilderness", "complete");
259+
int expectedTimes = PickFirstLoadBalancerProvider.isEnabledNewPickFirst() ? 1 : 2;
260+
verifyLongCounterAdd("grpc.lb.rls.target_picks", expectedTimes, 1, "wilderness", "complete");
256261

257262
// rescue should be pending status although the overall channel state is READY
258263
res = picker.pickSubchannel(rescueSubchannelArgs);
@@ -323,18 +328,22 @@ public void lb_working_withDefaultTarget_noRlsResponse() throws Exception {
323328
inOrder.verify(helper).getMetricRecorder();
324329
inOrder.verify(helper).getChannelTarget();
325330
inOrder.verifyNoMoreInteractions();
326-
verifyLongCounterAdd("grpc.lb.rls.default_target_picks", 1, 1, "defaultTarget", "complete");
331+
int times = PickFirstLoadBalancerProvider.isEnabledNewPickFirst() ? 1 : 2;
332+
verifyLongCounterAdd("grpc.lb.rls.default_target_picks", times, 1,
333+
"defaultTarget", "complete");
327334
verifyNoMoreInteractions(mockMetricRecorder);
328335

329336
Subchannel subchannel = picker.pickSubchannel(searchSubchannelArgs).getSubchannel();
330337
assertThat(subchannelIsReady(subchannel)).isTrue();
331338
assertThat(subchannel).isSameInstanceAs(fallbackSubchannel);
332-
verifyLongCounterAdd("grpc.lb.rls.default_target_picks", 2, 1, "defaultTarget", "complete");
339+
verifyLongCounterAdd("grpc.lb.rls.default_target_picks", ++times, 1, "defaultTarget",
340+
"complete");
333341

334342
subchannel = picker.pickSubchannel(searchSubchannelArgs).getSubchannel();
335343
assertThat(subchannelIsReady(subchannel)).isTrue();
336344
assertThat(subchannel).isSameInstanceAs(fallbackSubchannel);
337-
verifyLongCounterAdd("grpc.lb.rls.default_target_picks", 3, 1, "defaultTarget", "complete");
345+
verifyLongCounterAdd("grpc.lb.rls.default_target_picks", ++times, 1, "defaultTarget",
346+
"complete");
338347

339348
// Make sure that when RLS starts communicating that default stops being used
340349
fakeThrottler.nextResult = false;
@@ -345,7 +354,8 @@ public void lb_working_withDefaultTarget_noRlsResponse() throws Exception {
345354
(FakeSubchannel) markReadyAndGetPickResult(inOrder, searchSubchannelArgs).getSubchannel();
346355
assertThat(searchSubchannel).isNotNull();
347356
assertThat(searchSubchannel).isNotSameInstanceAs(fallbackSubchannel);
348-
verifyLongCounterAdd("grpc.lb.rls.target_picks", 1, 1, "wilderness", "complete");
357+
times = PickFirstLoadBalancerProvider.isEnabledNewPickFirst() ? 1 : 2;
358+
verifyLongCounterAdd("grpc.lb.rls.target_picks", times, 1, "wilderness", "complete");
349359

350360
// create rescue subchannel
351361
picker.pickSubchannel(rescueSubchannelArgs);
@@ -389,6 +399,8 @@ public void lb_working_withoutDefaultTarget() throws Exception {
389399
.updateBalancingState(eq(ConnectivityState.CONNECTING), any(SubchannelPicker.class));
390400
inOrder.verify(helper, atLeast(0)).getSynchronizationContext();
391401
inOrder.verify(helper, atLeast(0)).getScheduledExecutorService();
402+
inOrder.verify(helper, atLeast(0)).getMetricRecorder();
403+
inOrder.verify(helper, atLeast(0)).getChannelTarget();
392404
inOrder.verifyNoMoreInteractions();
393405
assertThat(res.getStatus().isOk()).isTrue();
394406

@@ -425,7 +437,8 @@ public void lb_working_withoutDefaultTarget() throws Exception {
425437
res = picker.pickSubchannel(newPickSubchannelArgs(fakeSearchMethod));
426438
assertThat(res.getStatus().isOk()).isFalse();
427439
assertThat(subchannelIsReady(res.getSubchannel())).isFalse();
428-
verifyLongCounterAdd("grpc.lb.rls.target_picks", 1, 1, "wilderness", "complete");
440+
int expectedTimes = PickFirstLoadBalancerProvider.isEnabledNewPickFirst() ? 1 : 2;
441+
verifyLongCounterAdd("grpc.lb.rls.target_picks", expectedTimes, 1, "wilderness", "complete");
429442

430443
res = picker.pickSubchannel(newPickSubchannelArgs(fakeRescueMethod));
431444
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
@@ -56,7 +56,7 @@
5656
import io.grpc.Status;
5757
import io.grpc.SynchronizationContext;
5858
import io.grpc.internal.FakeClock;
59-
import io.grpc.internal.GrpcUtil;
59+
import io.grpc.internal.PickFirstLoadBalancerProvider;
6060
import io.grpc.internal.TestUtils;
6161
import io.grpc.services.InternalCallMetricRecorder;
6262
import io.grpc.services.MetricReport;
@@ -579,7 +579,7 @@ weightedChild2.new OrcaReportListener(weightedConfig.errorUtilizationPenalty).on
579579
}
580580

581581
private boolean isEnabledHappyEyeballs() {
582-
return GrpcUtil.getFlag("GRPC_EXPERIMENTAL_XDS_DUALSTACK_ENDPOINTS", true);
582+
return PickFirstLoadBalancerProvider.isEnabledHappyEyeballs();
583583
}
584584

585585
@Test

0 commit comments

Comments
 (0)