Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Change HappyEyeballs and new pick first LB flags default value to false #11120

Merged
merged 4 commits into from
May 8, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -59,8 +59,6 @@ final class PickFirstLeafLoadBalancer extends LoadBalancer {
private static final Logger log = Logger.getLogger(PickFirstLeafLoadBalancer.class.getName());
@VisibleForTesting
static final int CONNECTION_DELAY_INTERVAL_MS = 250;
public static final String GRPC_EXPERIMENTAL_XDS_DUALSTACK_ENDPOINTS =
"GRPC_EXPERIMENTAL_XDS_DUALSTACK_ENDPOINTS";
private final Helper helper;
private final Map<SocketAddress, SubchannelData> subchannels = new HashMap<>();
private Index addressIndex;
Expand All @@ -71,7 +69,7 @@ final class PickFirstLeafLoadBalancer extends LoadBalancer {
private ConnectivityState rawConnectivityState = IDLE;
private ConnectivityState concludedState = IDLE;
private final boolean enableHappyEyeballs =
GrpcUtil.getFlag(GRPC_EXPERIMENTAL_XDS_DUALSTACK_ENDPOINTS, true);
PickFirstLoadBalancerProvider.isEnabledHappyEyeballs();

PickFirstLeafLoadBalancer(Helper helper) {
this.helper = checkNotNull(helper, "helper");
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -33,10 +33,21 @@
* down the address list and sticks to the first that works.
*/
public final class PickFirstLoadBalancerProvider extends LoadBalancerProvider {
public static final String GRPC_EXPERIMENTAL_XDS_DUALSTACK_ENDPOINTS =
"GRPC_EXPERIMENTAL_XDS_DUALSTACK_ENDPOINTS";
private static final String SHUFFLE_ADDRESS_LIST_KEY = "shuffleAddressList";

static boolean enableNewPickFirst =
GrpcUtil.getFlag("GRPC_EXPERIMENTAL_ENABLE_NEW_PICK_FIRST", true);
GrpcUtil.getFlag("GRPC_EXPERIMENTAL_ENABLE_NEW_PICK_FIRST", false);

public static boolean isEnabledHappyEyeballs() {
return GrpcUtil.getFlag(GRPC_EXPERIMENTAL_XDS_DUALSTACK_ENDPOINTS, false);
}

@VisibleForTesting
public static boolean isEnableNewPickFirst() {
return enableNewPickFirst;
}

@Override
public boolean isAvailable() {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -142,8 +142,8 @@ public void uncaughtException(Thread t, Throwable e) {
@Before
public void setUp() {
originalHappyEyeballsEnabledValue =
System.getProperty(PickFirstLeafLoadBalancer.GRPC_EXPERIMENTAL_XDS_DUALSTACK_ENDPOINTS);
System.setProperty(PickFirstLeafLoadBalancer.GRPC_EXPERIMENTAL_XDS_DUALSTACK_ENDPOINTS,
System.getProperty(PickFirstLoadBalancerProvider.GRPC_EXPERIMENTAL_XDS_DUALSTACK_ENDPOINTS);
System.setProperty(PickFirstLoadBalancerProvider.GRPC_EXPERIMENTAL_XDS_DUALSTACK_ENDPOINTS,
enableHappyEyeballs ? "true" : "false");

for (int i = 1; i <= 5; i++) {
Expand Down Expand Up @@ -173,9 +173,9 @@ public void setUp() {
@After
public void tearDown() {
if (originalHappyEyeballsEnabledValue == null) {
System.clearProperty(PickFirstLeafLoadBalancer.GRPC_EXPERIMENTAL_XDS_DUALSTACK_ENDPOINTS);
System.clearProperty(PickFirstLoadBalancerProvider.GRPC_EXPERIMENTAL_XDS_DUALSTACK_ENDPOINTS);
} else {
System.setProperty(PickFirstLeafLoadBalancer.GRPC_EXPERIMENTAL_XDS_DUALSTACK_ENDPOINTS,
System.setProperty(PickFirstLoadBalancerProvider.GRPC_EXPERIMENTAL_XDS_DUALSTACK_ENDPOINTS,
originalHappyEyeballsEnabledValue);
}

Expand Down
27 changes: 20 additions & 7 deletions rls/src/test/java/io/grpc/rls/RlsLoadBalancerTest.java
Original file line number Diff line number Diff line change
Expand Up @@ -70,6 +70,7 @@
import io.grpc.inprocess.InProcessServerBuilder;
import io.grpc.internal.FakeClock;
import io.grpc.internal.JsonParser;
import io.grpc.internal.PickFirstLoadBalancerProvider;
import io.grpc.internal.PickSubchannelArgsImpl;
import io.grpc.internal.testing.StreamRecorder;
import io.grpc.lookup.v1.RouteLookupServiceGrpc;
Expand Down Expand Up @@ -212,7 +213,8 @@ public void lb_serverStatusCodeConversion() throws Exception {
subchannel.updateState(ConnectivityStateInfo.forNonError(ConnectivityState.READY));
res = picker.pickSubchannel(fakeSearchMethodArgs);
assertThat(res.getStatus().getCode()).isEqualTo(Status.Code.OK);
verifyLongCounterAdd("grpc.lb.rls.target_picks", 1, 1, "wilderness", "complete");
int expectedTimes = PickFirstLoadBalancerProvider.isEnabledNewPickFirst() ? 1 : 2;
verifyLongCounterAdd("grpc.lb.rls.target_picks", expectedTimes, 1, "wilderness", "complete");

// Check on conversion
Throwable cause = new Throwable("cause");
Expand Down Expand Up @@ -244,6 +246,8 @@ public void lb_working_withDefaultTarget_rlsResponding() throws Exception {
.updateBalancingState(eq(ConnectivityState.CONNECTING), any(SubchannelPicker.class));
inOrder.verify(helper, atLeast(0)).getSynchronizationContext();
inOrder.verify(helper, atLeast(0)).getScheduledExecutorService();
inOrder.verify(helper, atLeast(0)).getMetricRecorder();
inOrder.verify(helper, atLeast(0)).getChannelTarget();
inOrder.verifyNoMoreInteractions();

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

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

Subchannel subchannel = picker.pickSubchannel(searchSubchannelArgs).getSubchannel();
assertThat(subchannelIsReady(subchannel)).isTrue();
assertThat(subchannel).isSameInstanceAs(fallbackSubchannel);
verifyLongCounterAdd("grpc.lb.rls.default_target_picks", 2, 1, "defaultTarget", "complete");
verifyLongCounterAdd("grpc.lb.rls.default_target_picks", ++times, 1, "defaultTarget",
"complete");

subchannel = picker.pickSubchannel(searchSubchannelArgs).getSubchannel();
assertThat(subchannelIsReady(subchannel)).isTrue();
assertThat(subchannel).isSameInstanceAs(fallbackSubchannel);
verifyLongCounterAdd("grpc.lb.rls.default_target_picks", 3, 1, "defaultTarget", "complete");
verifyLongCounterAdd("grpc.lb.rls.default_target_picks", ++times, 1, "defaultTarget",
"complete");

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

// create rescue subchannel
picker.pickSubchannel(rescueSubchannelArgs);
Expand Down Expand Up @@ -433,6 +443,8 @@ public void lb_working_withoutDefaultTarget() throws Exception {
.updateBalancingState(eq(ConnectivityState.CONNECTING), any(SubchannelPicker.class));
inOrder.verify(helper, atLeast(0)).getSynchronizationContext();
inOrder.verify(helper, atLeast(0)).getScheduledExecutorService();
inOrder.verify(helper, atLeast(0)).getMetricRecorder();
inOrder.verify(helper, atLeast(0)).getChannelTarget();
inOrder.verifyNoMoreInteractions();
assertThat(res.getStatus().isOk()).isTrue();

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

res = picker.pickSubchannel(newPickSubchannelArgs(fakeRescueMethod));
assertThat(subchannelIsReady(res.getSubchannel())).isTrue();
Expand Down
2 changes: 1 addition & 1 deletion xds/src/main/java/io/grpc/xds/XdsEndpointResource.java
Original file line number Diff line number Diff line change
Expand Up @@ -101,7 +101,7 @@ protected EdsUpdate doParse(Args args, Message unpackedMessage) throws ResourceI
}

private static boolean isEnabledXdsDualStack() {
return GrpcUtil.getFlag(GRPC_EXPERIMENTAL_XDS_DUALSTACK_ENDPOINTS, true);
return GrpcUtil.getFlag(GRPC_EXPERIMENTAL_XDS_DUALSTACK_ENDPOINTS, false);
}

private static EdsUpdate processClusterLoadAssignment(ClusterLoadAssignment assignment)
Expand Down
10 changes: 7 additions & 3 deletions xds/src/test/java/io/grpc/xds/RingHashLoadBalancerTest.java
Original file line number Diff line number Diff line change
Expand Up @@ -151,7 +151,8 @@ public void subchannelLazyConnectUntilPicked() {
assertThat(result.getStatus().isOk()).isTrue();
assertThat(result.getSubchannel()).isNull();
Subchannel subchannel = Iterables.getOnlyElement(subchannels.values());
verify(subchannel).requestConnection();
int expectedTimes = PickFirstLoadBalancerProvider.isEnabledHappyEyeballs() ? 1 : 2;
verify(subchannel, times(expectedTimes)).requestConnection();
verify(helper).updateBalancingState(eq(CONNECTING), any(SubchannelPicker.class));
verify(helper).createSubchannel(any(CreateSubchannelArgs.class));
deliverSubchannelState(subchannel, CSI_CONNECTING);
Expand Down Expand Up @@ -185,7 +186,8 @@ public void subchannelNotAutoReconnectAfterReenteringIdle() {
pickerCaptor.getValue().pickSubchannel(args);
Subchannel subchannel = subchannels.get(Collections.singletonList(childLbState.getEag()));
InOrder inOrder = Mockito.inOrder(helper, subchannel);
inOrder.verify(subchannel).requestConnection();
int expectedTimes = PickFirstLoadBalancerProvider.isEnabledHappyEyeballs() ? 1 : 2;
inOrder.verify(subchannel, times(expectedTimes)).requestConnection();
deliverSubchannelState(subchannel, CSI_READY);
inOrder.verify(helper).updateBalancingState(eq(READY), any(SubchannelPicker.class));
deliverSubchannelState(subchannel, ConnectivityStateInfo.forNonError(IDLE));
Expand Down Expand Up @@ -443,7 +445,9 @@ public void skipFailingHosts_pickNextNonFailingHost() {
PickResult result = pickerCaptor.getValue().pickSubchannel(args);
assertThat(result.getStatus().isOk()).isTrue();
assertThat(result.getSubchannel()).isNull(); // buffer request
verify(getSubChannel(servers.get(1))).requestConnection(); // kicked off connection to server2
int expectedTimes = PickFirstLoadBalancerProvider.isEnabledHappyEyeballs() ? 1 : 2;
// verify kicked off connection to server2
verify(getSubChannel(servers.get(1)), times(expectedTimes)).requestConnection();
assertThat(subchannels.size()).isEqualTo(2); // no excessive connection

deliverSubchannelState(getSubChannel(servers.get(1)), CSI_CONNECTING);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -56,7 +56,7 @@
import io.grpc.Status;
import io.grpc.SynchronizationContext;
import io.grpc.internal.FakeClock;
import io.grpc.internal.GrpcUtil;
import io.grpc.internal.PickFirstLoadBalancerProvider;
import io.grpc.internal.TestUtils;
import io.grpc.services.InternalCallMetricRecorder;
import io.grpc.services.MetricReport;
Expand Down Expand Up @@ -580,7 +580,7 @@ weightedChild2.new OrcaReportListener(weightedConfig.errorUtilizationPenalty).on
}

private boolean isEnabledHappyEyeballs() {
return GrpcUtil.getFlag("GRPC_EXPERIMENTAL_XDS_DUALSTACK_ENDPOINTS", true);
return PickFirstLoadBalancerProvider.isEnabledHappyEyeballs();
}

@Test
Expand Down