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.

Sign up for GitHub

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

Enable Happy Eyeballs by default #11022

Merged
merged 5 commits into from
Mar 21, 2024
Merged

Enable Happy Eyeballs by default #11022

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
54b0ba6
Enable Happy Eyeballs by default
larry-safran Mar 15, 2024
7ba6032
Fix test expectations when Happy Eyeballs is enabled.
larry-safran Mar 15, 2024
22b49d6
Clarify comment
larry-safran Mar 21, 2024
7c2f8b5
Fix test flakiness where IPv6 was not considered loopback
larry-safran Mar 21, 2024
006f8c6
Checkstyle
larry-safran Mar 21, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 11 additions & 2 deletions core/src/main/java/io/grpc/internal/PickFirstLeafLoadBalancer.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@
import io.grpc.ExperimentalApi;
import io.grpc.LoadBalancer;
import io.grpc.Status;
import io.grpc.SynchronizationContext;
import io.grpc.SynchronizationContext.ScheduledHandle;
import java.net.SocketAddress;
import java.util.ArrayList;
Expand Down Expand Up @@ -72,7 +73,7 @@
private ConnectivityState rawConnectivityState = IDLE;
private ConnectivityState concludedState = IDLE;
private final boolean enableHappyEyeballs =
GrpcUtil.getFlag(GRPC_EXPERIMENTAL_XDS_DUALSTACK_ENDPOINTS, false);
GrpcUtil.getFlag(GRPC_EXPERIMENTAL_XDS_DUALSTACK_ENDPOINTS, true);

PickFirstLeafLoadBalancer(Helper helper) {
this.helper = checkNotNull(helper, "helper");
Expand Down Expand Up @@ -406,7 +407,15 @@
}
}

scheduleConnectionTask = helper.getSynchronizationContext().schedule(
SynchronizationContext synchronizationContext = null;
try {
synchronizationContext = helper.getSynchronizationContext();
} catch (NullPointerException e) {

Check warning on line 413 in core/src/main/java/io/grpc/internal/PickFirstLeafLoadBalancer.java

View check run for this annotation

Codecov / codecov/patch

core/src/main/java/io/grpc/internal/PickFirstLeafLoadBalancer.java#L413 

Added line #L413 was not covered by tests
// If we have a test helper or old helper without a context, we can't schedule a task
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we fix the test instead of the code?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did fix the tests to provide a synchronization context, but customers could be relying on an old helper that they don't expect to suddenly start failing, so we need to handle the case.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The test I can believe it can be not implemented. But don't mention "test helper" in code comment.
The LB helper should always have getSynchronizationContext(), from the customer. Originally it is from ManagedChannelImpl. Where is the old helper that does not have this?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've updated the comment. Despite not having any known examples that are not providing the sync context I don't want to break something unknown out in the wild.

return;

Check warning on line 415 in core/src/main/java/io/grpc/internal/PickFirstLeafLoadBalancer.java

View check run for this annotation

Codecov / codecov/patch

core/src/main/java/io/grpc/internal/PickFirstLeafLoadBalancer.java#L415 

Added line #L415 was not covered by tests
}

scheduleConnectionTask = synchronizationContext.schedule(
new StartNextConnection(),
CONNECTION_DELAY_INTERVAL_MS,
TimeUnit.MILLISECONDS,
Expand Down
22 changes: 22 additions & 0 deletions core/src/test/java/io/grpc/internal/AutoConfiguredLoadBalancerFactoryTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,7 @@
import io.grpc.LoadBalancerRegistry;
import io.grpc.NameResolver.ConfigOrError;
import io.grpc.Status;
import io.grpc.SynchronizationContext;
import io.grpc.internal.AutoConfiguredLoadBalancerFactory.AutoConfiguredLoadBalancer;
import io.grpc.internal.PickFirstLeafLoadBalancer.PickFirstLeafLoadBalancerConfig;
import io.grpc.internal.PickFirstLoadBalancer.PickFirstLoadBalancerConfig;
Expand All @@ -58,6 +59,7 @@
import java.util.Collections;
import java.util.List;
import java.util.Map;
import java.util.concurrent.ScheduledExecutorService;
import java.util.concurrent.atomic.AtomicBoolean;
import java.util.concurrent.atomic.AtomicInteger;
import java.util.concurrent.atomic.AtomicReference;
Expand Down Expand Up @@ -691,6 +693,16 @@ private static class TestLoadBalancer extends ForwardingLoadBalancer {
}

private class TestHelper extends ForwardingLoadBalancerHelper {
final SynchronizationContext syncContext = new SynchronizationContext(
new Thread.UncaughtExceptionHandler() {
@Override
public void uncaughtException(Thread t, Throwable e) {
throw new AssertionError(e);
}
});

final FakeClock fakeClock = new FakeClock();

@Override
protected Helper delegate() {
return null;
Expand All @@ -705,6 +717,16 @@ public ChannelLogger getChannelLogger() {
public void updateBalancingState(ConnectivityState newState, SubchannelPicker newPicker) {
// noop
}

@Override
public SynchronizationContext getSynchronizationContext() {
return syncContext;
}

@Override
public ScheduledExecutorService getScheduledExecutorService() {
return fakeClock.getScheduledExecutorService();
}
}

private static class TestSubchannel extends Subchannel {
Expand Down
4 changes: 4 additions & 0 deletions rls/src/test/java/io/grpc/rls/RlsLoadBalancerTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -217,6 +217,8 @@ public void lb_working_withDefaultTarget_rlsResponding() throws Exception {
inOrder.verify(helper).createSubchannel(any(CreateSubchannelArgs.class));
inOrder.verify(helper, atLeast(0))
.updateBalancingState(eq(ConnectivityState.CONNECTING), any(SubchannelPicker.class));
inOrder.verify(helper, atLeast(0)).getSynchronizationContext();
inOrder.verify(helper, atLeast(0)).getScheduledExecutorService();
inOrder.verifyNoMoreInteractions();
assertThat(res.getStatus().isOk()).isTrue();
assertThat(subchannels).hasSize(1);
Expand Down Expand Up @@ -325,6 +327,8 @@ public void lb_working_withoutDefaultTarget() throws Exception {
inOrder.verify(helper).createSubchannel(any(CreateSubchannelArgs.class));
inOrder.verify(helper, atLeast(0))
.updateBalancingState(eq(ConnectivityState.CONNECTING), any(SubchannelPicker.class));
inOrder.verify(helper, atLeast(0)).getSynchronizationContext();
inOrder.verify(helper, atLeast(0)).getScheduledExecutorService();
inOrder.verifyNoMoreInteractions();
assertThat(res.getStatus().isOk()).isTrue();

Expand Down
5 changes: 2 additions & 3 deletions util/src/test/java/io/grpc/util/MultiChildLoadBalancerTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,6 @@
import static org.mockito.Mockito.never;
import static org.mockito.Mockito.times;
import static org.mockito.Mockito.verify;
import static org.mockito.Mockito.verifyNoMoreInteractions;

import com.google.common.collect.Lists;
import io.grpc.Attributes;
Expand Down Expand Up @@ -128,7 +127,7 @@ public void pickAfterResolved() {
(TestLb.TestSubchannelPicker) pickerCaptor.getValue();
assertThat(subchannelPicker.getReadySubchannels()).containsExactly(readySubchannel);

verifyNoMoreInteractions(mockHelper);
AbstractTestHelper.verifyNoMoreMeaningfulInteractions(mockHelper);
}

@Test
Expand Down Expand Up @@ -192,7 +191,7 @@ public void pickAfterResolvedUpdatedHosts() {
verify(mockHelper, times(3)).createSubchannel(any(LoadBalancer.CreateSubchannelArgs.class));
inOrder.verify(mockHelper, times(2)).updateBalancingState(eq(READY), pickerCaptor.capture());

verifyNoMoreInteractions(mockHelper);
AbstractTestHelper.verifyNoMoreMeaningfulInteractions(mockHelper);
}

@Test
Expand Down
10 changes: 5 additions & 5 deletions util/src/test/java/io/grpc/util/RoundRobinLoadBalancerTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -153,7 +153,7 @@ public void pickAfterResolved() throws Exception {
assertEquals(READY, stateCaptor.getAllValues().get(1));
assertThat(getList(pickerCaptor.getValue())).containsExactly(readySubchannel);

verifyNoMoreInteractions(mockHelper);
AbstractTestHelper.verifyNoMoreMeaningfulInteractions(mockHelper);
}

@Test
Expand Down Expand Up @@ -234,7 +234,7 @@ public void pickAfterResolvedUpdatedHosts() throws Exception {
picker = pickerCaptor.getValue();
assertThat(getList(picker)).containsExactly(oldSubchannel, newSubchannel);

verifyNoMoreInteractions(mockHelper);
AbstractTestHelper.verifyNoMoreMeaningfulInteractions(mockHelper);
}

@Test
Expand Down Expand Up @@ -269,7 +269,7 @@ public void pickAfterStateChange() throws Exception {

verify(subchannel, atLeastOnce()).requestConnection();
verify(mockHelper, times(3)).createSubchannel(any(CreateSubchannelArgs.class));
verifyNoMoreInteractions(mockHelper);
AbstractTestHelper.verifyNoMoreMeaningfulInteractions(mockHelper);
}

@Test
Expand Down Expand Up @@ -355,7 +355,7 @@ public void refreshNameResolutionWhenSubchannelConnectionBroken() {
inOrder.verify(mockHelper).updateBalancingState(eq(CONNECTING), isA(EmptyPicker.class));
}

verifyNoMoreInteractions(mockHelper);
AbstractTestHelper.verifyNoMoreMeaningfulInteractions(mockHelper);
}

@Test
Expand Down Expand Up @@ -432,7 +432,7 @@ public void nameResolutionErrorWithActiveChannels() throws Exception {

LoadBalancer.PickResult pickResult2 = pickerCaptor.getValue().pickSubchannel(mockArgs);
assertEquals(readySubchannel, pickResult2.getSubchannel());
verifyNoMoreInteractions(mockHelper);
AbstractTestHelper.verifyNoMoreMeaningfulInteractions(mockHelper);
}

@Test
Expand Down
56 changes: 56 additions & 0 deletions util/src/testFixtures/java/io/grpc/util/AbstractTestHelper.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,10 @@

import static org.mockito.AdditionalAnswers.delegatesTo;
import static org.mockito.ArgumentMatchers.eq;
import static org.mockito.Mockito.atLeast;
import static org.mockito.Mockito.mock;
import static org.mockito.Mockito.verify;
import static org.mockito.Mockito.verifyNoMoreInteractions;

import com.google.common.collect.Maps;
import io.grpc.Attributes;
Expand All @@ -32,12 +35,15 @@
import io.grpc.LoadBalancer.Subchannel;
import io.grpc.LoadBalancer.SubchannelPicker;
import io.grpc.LoadBalancer.SubchannelStateListener;
import io.grpc.SynchronizationContext;
import io.grpc.internal.FakeClock;
import io.grpc.internal.PickFirstLoadBalancerProvider;
import java.net.SocketAddress;
import java.util.Collections;
import java.util.HashMap;
import java.util.List;
import java.util.Map;
import java.util.concurrent.ScheduledExecutorService;
import org.mockito.ArgumentCaptor;
import org.mockito.InOrder;

Expand All @@ -60,9 +66,26 @@ public abstract class AbstractTestHelper extends ForwardingLoadBalancerHelper {
protected final Map<Subchannel, Subchannel> realToMockSubChannelMap = new HashMap<>();
private final Map<Subchannel, SubchannelStateListener> subchannelStateListeners =
Maps.newLinkedHashMap();
private final FakeClock fakeClock;
private final SynchronizationContext syncContext;

public abstract Map<List<EquivalentAddressGroup>, Subchannel> getSubchannelMap();

public AbstractTestHelper() {
this(new FakeClock(), new SynchronizationContext(new Thread.UncaughtExceptionHandler() {
@Override
public void uncaughtException(Thread t, Throwable e) {
throw new RuntimeException(e);
}
}));
}

public AbstractTestHelper(FakeClock fakeClock, SynchronizationContext syncContext) {
super();
this.fakeClock = fakeClock;
this.syncContext = syncContext;
}

public Map<Subchannel, Subchannel> getMockToRealSubChannelMap() {
return mockToRealSubChannelMap;
}
Expand All @@ -79,6 +102,18 @@ public Map<Subchannel, SubchannelStateListener> getSubchannelStateListeners() {
return subchannelStateListeners;
}

public static final FakeClock.TaskFilter NOT_START_NEXT_CONNECTION =
new FakeClock.TaskFilter() {
@Override
public boolean shouldAccept(Runnable command) {
return !command.toString().contains("StartNextConnection");
}
};

public static int getNumFilteredPendingTasks(FakeClock fakeClock) {
return fakeClock.getPendingTasks(NOT_START_NEXT_CONNECTION).size();
}

public void deliverSubchannelState(Subchannel subchannel, ConnectivityStateInfo newState) {
Subchannel realSc = getMockToRealSubChannelMap().get(subchannel);
if (realSc == null) {
Expand Down Expand Up @@ -128,6 +163,16 @@ public void setChannel(Subchannel subchannel, Channel channel) {
((TestSubchannel)subchannel).channel = channel;
}

@Override
public SynchronizationContext getSynchronizationContext() {
return syncContext;
}

@Override
public ScheduledExecutorService getScheduledExecutorService() {
return fakeClock.getScheduledExecutorService();
}

@Override
public String toString() {
return "Test Helper";
Expand All @@ -148,6 +193,17 @@ public static void refreshInvokedAndUpdateBS(InOrder inOrder, ConnectivityState
}
}

public static void verifyNoMoreMeaningfulInteractions(Helper helper) {
verify(helper, atLeast(0)).getSynchronizationContext();
verify(helper, atLeast(0)).getScheduledExecutorService();
verifyNoMoreInteractions(helper);
}

public static void verifyNoMoreMeaningfulInteractions(Helper helper, InOrder inOrder) {
inOrder.verify(helper, atLeast(0)).getSynchronizationContext();
inOrder.verify(helper, atLeast(0)).getScheduledExecutorService();
inOrder.verifyNoMoreInteractions();
}

protected class TestSubchannel extends ForwardingSubchannel {
CreateSubchannelArgs args;
Expand Down
2 changes: 1 addition & 1 deletion xds/src/main/java/io/grpc/xds/XdsEndpointResource.java
View file
Open in desktop
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, false);
return GrpcUtil.getFlag(GRPC_EXPERIMENTAL_XDS_DUALSTACK_ENDPOINTS, true);
}

private static EdsUpdate processClusterLoadAssignment(ClusterLoadAssignment assignment)
Expand Down
14 changes: 7 additions & 7 deletions xds/src/test/java/io/grpc/xds/LeastRequestLoadBalancerTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -159,7 +159,7 @@ public void pickAfterResolved() throws Exception {
assertEquals(READY, stateCaptor.getAllValues().get(1));
assertThat(getList(pickerCaptor.getValue())).containsExactly(readySubchannel);

verifyNoMoreInteractions(helper);
AbstractTestHelper.verifyNoMoreMeaningfulInteractions(helper);
}

@Test
Expand Down Expand Up @@ -226,7 +226,7 @@ public void pickAfterResolvedUpdatedHosts() throws Exception {

assertThat(getList(pickerCaptor.getValue())).containsExactly(oldSubchannel, newSubchannel);

verifyNoMoreInteractions(helper);
AbstractTestHelper.verifyNoMoreMeaningfulInteractions(helper);
}

private Subchannel getSubchannel(EquivalentAddressGroup removedEag) {
Expand Down Expand Up @@ -288,7 +288,7 @@ public void pickAfterStateChange() throws Exception {
int expectedCount = PickFirstLoadBalancerProvider.isEnabledNewPickFirst() ? 1 : 2;
verify(subchannel, times(expectedCount)).requestConnection();
verify(helper, times(3)).createSubchannel(any(CreateSubchannelArgs.class));
verifyNoMoreInteractions(helper);
AbstractTestHelper.verifyNoMoreMeaningfulInteractions(helper);
}

@Test
Expand Down Expand Up @@ -319,7 +319,7 @@ public void pickAfterConfigChange() {
// At this point it should use a ReadyPicker with newConfig
pickerCaptor.getValue().pickSubchannel(mockArgs);
verify(mockRandom, times(oldConfig.choiceCount + newConfig.choiceCount)).nextInt(1);
verifyNoMoreInteractions(helper);
AbstractTestHelper.verifyNoMoreMeaningfulInteractions(helper);
}

@Test
Expand Down Expand Up @@ -375,7 +375,7 @@ public void stayTransientFailureUntilReady() {
inOrder.verify(helper).updateBalancingState(eq(READY), isA(ReadyPicker.class));

verify(helper, times(3)).createSubchannel(any(CreateSubchannelArgs.class));
verifyNoMoreInteractions(helper);
AbstractTestHelper.verifyNoMoreMeaningfulInteractions(helper);
}

private String getStatusString(SubchannelPicker picker) {
Expand Down Expand Up @@ -426,7 +426,7 @@ public void refreshNameResolutionWhenSubchannelConnectionBroken() {
inOrder.verify(helper).updateBalancingState(eq(CONNECTING), isA(EmptyPicker.class));
}

verifyNoMoreInteractions(helper);
AbstractTestHelper.verifyNoMoreMeaningfulInteractions(helper);
}

@Test
Expand Down Expand Up @@ -548,7 +548,7 @@ public void nameResolutionErrorWithActiveChannels() throws Exception {
LoadBalancer.PickResult pickResult2 = pickerCaptor.getValue().pickSubchannel(mockArgs);
verify(mockRandom, times(choiceCount * 2)).nextInt(1);
assertEquals(readySubchannel, pickResult2.getSubchannel());
verifyNoMoreInteractions(helper);
AbstractTestHelper.verifyNoMoreMeaningfulInteractions(helper);
}

@Test
Expand Down
11 changes: 5 additions & 6 deletions xds/src/test/java/io/grpc/xds/RingHashLoadBalancerTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,7 @@
import io.grpc.Status;
import io.grpc.Status.Code;
import io.grpc.SynchronizationContext;
import io.grpc.internal.FakeClock;
import io.grpc.internal.PickFirstLoadBalancerProvider;
import io.grpc.internal.PickSubchannelArgsImpl;
import io.grpc.testing.TestMethodDescriptors;
Expand Down Expand Up @@ -161,7 +162,7 @@ public void subchannelLazyConnectUntilPicked() {
verify(helper).updateBalancingState(eq(READY), pickerCaptor.capture());
result = pickerCaptor.getValue().pickSubchannel(args);
assertThat(result.getSubchannel()).isSameInstanceAs(subchannel);
verifyNoMoreInteractions(helper);
AbstractTestHelper.verifyNoMoreMeaningfulInteractions(helper);
}

@Test
Expand Down Expand Up @@ -1055,6 +1056,9 @@ public String toString() {
}

private class TestHelper extends AbstractTestHelper {
public TestHelper() {
super(new FakeClock(), syncContext);
}

@Override
public Map<List<EquivalentAddressGroup>, Subchannel> getSubchannelMap() {
Expand All @@ -1066,11 +1070,6 @@ public String getAuthority() {
return AUTHORITY;
}

@Override
public SynchronizationContext getSynchronizationContext() {
return syncContext;
}

private Subchannel getMockSubchannel(Subchannel realSubchannel) {
return realToMockSubChannelMap.get(realSubchannel);
}
Expand Down
Loading
Loading