Skip to content

Prefer FixedResultPicker over custom Picker implementations #12590

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

Open
ejona86 wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Prefer FixedResultPicker over custom Picker implementations #12590

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
842c53f
Prefer FixedResultPicker over custom Picker implementations
ejona86 Nov 19, 2025
9287f85
Fix grpclb incompatibility
ejona86 Jan 3, 2026
faf0add
Fix RLS test
ejona86 Jan 3, 2026
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
36 changes: 5 additions & 31 deletions core/src/main/java/io/grpc/internal/AutoConfiguredLoadBalancerFactory.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,17 +19,15 @@
import static com.google.common.base.Preconditions.checkNotNull;

import com.google.common.annotations.VisibleForTesting;
import com.google.common.base.MoreObjects;
import io.grpc.ChannelLogger.ChannelLogLevel;
import io.grpc.ConnectivityState;
import io.grpc.ConnectivityStateInfo;
import io.grpc.LoadBalancer;
import io.grpc.LoadBalancer.FixedResultPicker;
import io.grpc.LoadBalancer.Helper;
import io.grpc.LoadBalancer.PickResult;
import io.grpc.LoadBalancer.PickSubchannelArgs;
import io.grpc.LoadBalancer.ResolvedAddresses;
import io.grpc.LoadBalancer.Subchannel;
import io.grpc.LoadBalancer.SubchannelPicker;
import io.grpc.LoadBalancerProvider;
import io.grpc.LoadBalancerRegistry;
import io.grpc.NameResolver.ConfigOrError;
Expand Down Expand Up @@ -110,7 +108,8 @@ Status tryAcceptResolvedAddresses(ResolvedAddresses resolvedAddresses) {
defaultProvider = getProviderOrThrow(defaultPolicy, "using default policy");
} catch (PolicyException e) {
Status s = Status.INTERNAL.withDescription(e.getMessage());
helper.updateBalancingState(ConnectivityState.TRANSIENT_FAILURE, new FailingPicker(s));
helper.updateBalancingState(
ConnectivityState.TRANSIENT_FAILURE, new FixedResultPicker(PickResult.withError(s)));
delegate.shutdown();
delegateProvider = null;
delegate = new NoopLoadBalancer();
Expand All @@ -122,7 +121,8 @@ Status tryAcceptResolvedAddresses(ResolvedAddresses resolvedAddresses) {

if (delegateProvider == null
|| !policySelection.provider.getPolicyName().equals(delegateProvider.getPolicyName())) {
helper.updateBalancingState(ConnectivityState.CONNECTING, new EmptyPicker());
helper.updateBalancingState(
ConnectivityState.CONNECTING, new FixedResultPicker(PickResult.withNoResult()));
delegate.shutdown();
delegateProvider = policySelection.provider;
LoadBalancer old = delegate;
Expand Down Expand Up @@ -236,30 +236,4 @@ private PolicyException(String msg) {
super(msg);
}
}

private static final class EmptyPicker extends SubchannelPicker {

@Override
public PickResult pickSubchannel(PickSubchannelArgs args) {
return PickResult.withNoResult();
}

@Override
public String toString() {
return MoreObjects.toStringHelper(EmptyPicker.class).toString();
}
}

private static final class FailingPicker extends SubchannelPicker {
private final Status failure;

FailingPicker(Status failure) {
this.failure = failure;
}

@Override
public PickResult pickSubchannel(PickSubchannelArgs args) {
return PickResult.withError(failure);
}
}
}
39 changes: 4 additions & 35 deletions core/src/main/java/io/grpc/internal/OobChannel.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,8 +38,8 @@
import io.grpc.InternalLogId;
import io.grpc.InternalWithLogId;
import io.grpc.LoadBalancer;
import io.grpc.LoadBalancer.FixedResultPicker;
import io.grpc.LoadBalancer.PickResult;
import io.grpc.LoadBalancer.PickSubchannelArgs;
import io.grpc.LoadBalancer.Subchannel;
import io.grpc.LoadBalancer.SubchannelPicker;
import io.grpc.ManagedChannel;
Expand Down Expand Up @@ -182,23 +182,7 @@ public Object getInternalSubchannel() {
}
};

final class OobSubchannelPicker extends SubchannelPicker {
final PickResult result = PickResult.withSubchannel(subchannelImpl);

@Override
public PickResult pickSubchannel(PickSubchannelArgs args) {
return result;
}

@Override
public String toString() {
return MoreObjects.toStringHelper(OobSubchannelPicker.class)
.add("result", result)
.toString();
}
}

subchannelPicker = new OobSubchannelPicker();
subchannelPicker = new FixedResultPicker(PickResult.withSubchannel(subchannelImpl));
delayedTransport.reprocess(subchannelPicker);
}

Expand Down Expand Up @@ -270,23 +254,8 @@ void handleSubchannelStateChange(final ConnectivityStateInfo newState) {
delayedTransport.reprocess(subchannelPicker);
break;
case TRANSIENT_FAILURE:
final class OobErrorPicker extends SubchannelPicker {
final PickResult errorResult = PickResult.withError(newState.getStatus());

@Override
public PickResult pickSubchannel(PickSubchannelArgs args) {
return errorResult;
}

@Override
public String toString() {
return MoreObjects.toStringHelper(OobErrorPicker.class)
.add("errorResult", errorResult)
.toString();
}
}

delayedTransport.reprocess(new OobErrorPicker());
delayedTransport.reprocess(
new FixedResultPicker(PickResult.withError(newState.getStatus())));
break;
default:
// Do nothing
Expand Down
35 changes: 6 additions & 29 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 @@ -24,7 +24,6 @@
import static io.grpc.ConnectivityState.TRANSIENT_FAILURE;

import com.google.common.annotations.VisibleForTesting;
import com.google.common.base.MoreObjects;
import com.google.common.collect.ImmutableList;
import com.google.common.collect.Lists;
import io.grpc.Attributes;
Expand Down Expand Up @@ -164,7 +163,7 @@ public Status acceptResolvedAddresses(ResolvedAddresses resolvedAddresses) {
if (noOldAddrs) {
// Make tests happy; they don't properly assume starting in CONNECTING
rawConnectivityState = CONNECTING;
updateBalancingState(CONNECTING, new Picker(PickResult.withNoResult()));
updateBalancingState(CONNECTING, new FixedResultPicker(PickResult.withNoResult()));
}

if (rawConnectivityState == READY) {
Expand Down Expand Up @@ -237,7 +236,7 @@ public void handleNameResolutionError(Status error) {
subchannels.clear();
addressIndex.updateGroups(ImmutableList.of());
rawConnectivityState = TRANSIENT_FAILURE;
updateBalancingState(TRANSIENT_FAILURE, new Picker(PickResult.withError(error)));
updateBalancingState(TRANSIENT_FAILURE, new FixedResultPicker(PickResult.withError(error)));
}

void processSubchannelState(SubchannelData subchannelData, ConnectivityStateInfo stateInfo) {
Expand Down Expand Up @@ -290,7 +289,7 @@ void processSubchannelState(SubchannelData subchannelData, ConnectivityStateInfo

case CONNECTING:
rawConnectivityState = CONNECTING;
updateBalancingState(CONNECTING, new Picker(PickResult.withNoResult()));
updateBalancingState(CONNECTING, new FixedResultPicker(PickResult.withNoResult()));
break;

case READY:
Expand Down Expand Up @@ -322,7 +321,7 @@ void processSubchannelState(SubchannelData subchannelData, ConnectivityStateInfo
if (isPassComplete()) {
rawConnectivityState = TRANSIENT_FAILURE;
updateBalancingState(TRANSIENT_FAILURE,
new Picker(PickResult.withError(stateInfo.getStatus())));
new FixedResultPicker(PickResult.withError(stateInfo.getStatus())));

// Refresh Name Resolution, but only when all 3 conditions are met
// * We are at the end of addressIndex
Expand Down Expand Up @@ -385,11 +384,11 @@ private void updateHealthCheckedState(SubchannelData subchannelData) {
updateBalancingState(READY,
new FixedResultPicker(PickResult.withSubchannel(subchannelData.subchannel)));
} else if (subchannelData.getHealthState() == TRANSIENT_FAILURE) {
updateBalancingState(TRANSIENT_FAILURE, new Picker(PickResult.withError(
updateBalancingState(TRANSIENT_FAILURE, new FixedResultPicker(PickResult.withError(
subchannelData.healthStateInfo.getStatus())));
} else if (concludedState != TRANSIENT_FAILURE) {
updateBalancingState(subchannelData.getHealthState(),
new Picker(PickResult.withNoResult()));
new FixedResultPicker(PickResult.withNoResult()));
}
}

Expand Down Expand Up @@ -593,28 +592,6 @@ ConnectivityState getConcludedConnectivityState() {
return this.concludedState;
}

/**
* No-op picker which doesn't add any custom picking logic. It just passes already known result
* received in constructor.
*/
private static final class Picker extends SubchannelPicker {
private final PickResult result;

Picker(PickResult result) {
this.result = checkNotNull(result, "result");
}

@Override
public PickResult pickSubchannel(PickSubchannelArgs args) {
return result;
}

@Override
public String toString() {
return MoreObjects.toStringHelper(Picker.class).add("result", result).toString();
}
}

/**
* Picker that requests connection during the first pick, and returns noResult.
*/
Expand Down
33 changes: 5 additions & 28 deletions core/src/main/java/io/grpc/internal/PickFirstLoadBalancer.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,6 @@
import static io.grpc.ConnectivityState.SHUTDOWN;
import static io.grpc.ConnectivityState.TRANSIENT_FAILURE;

import com.google.common.base.MoreObjects;
import io.grpc.ConnectivityState;
import io.grpc.ConnectivityStateInfo;
import io.grpc.EquivalentAddressGroup;
Expand Down Expand Up @@ -87,7 +86,7 @@ public void onSubchannelState(ConnectivityStateInfo stateInfo) {

// The channel state does not get updated when doing name resolving today, so for the moment
// let LB report CONNECTION and call subchannel.requestConnection() immediately.
updateBalancingState(CONNECTING, new Picker(PickResult.withSubchannel(subchannel)));
updateBalancingState(CONNECTING, new FixedResultPicker(PickResult.withNoResult()));
subchannel.requestConnection();
} else {
subchannel.updateAddresses(servers);
Expand All @@ -105,7 +104,7 @@ public void handleNameResolutionError(Status error) {

// NB(lukaszx0) Whether we should propagate the error unconditionally is arguable. It's fine
// for time being.
updateBalancingState(TRANSIENT_FAILURE, new Picker(PickResult.withError(error)));
updateBalancingState(TRANSIENT_FAILURE, new FixedResultPicker(PickResult.withError(error)));
}

private void processSubchannelState(Subchannel subchannel, ConnectivityStateInfo stateInfo) {
Expand Down Expand Up @@ -139,13 +138,13 @@ private void processSubchannelState(Subchannel subchannel, ConnectivityStateInfo
case CONNECTING:
// It's safe to use RequestConnectionPicker here, so when coming from IDLE we could leave
// the current picker in-place. But ignoring the potential optimization is simpler.
picker = new Picker(PickResult.withNoResult());
picker = new FixedResultPicker(PickResult.withNoResult());
break;
case READY:
picker = new Picker(PickResult.withSubchannel(subchannel));
picker = new FixedResultPicker(PickResult.withSubchannel(subchannel));
break;
case TRANSIENT_FAILURE:
picker = new Picker(PickResult.withError(stateInfo.getStatus()));
picker = new FixedResultPicker(PickResult.withError(stateInfo.getStatus()));
break;
default:
throw new IllegalArgumentException("Unsupported state:" + newState);
Expand Down Expand Up @@ -173,28 +172,6 @@ public void requestConnection() {
}
}

/**
* No-op picker which doesn't add any custom picking logic. It just passes already known result
* received in constructor.
*/
private static final class Picker extends SubchannelPicker {
private final PickResult result;

Picker(PickResult result) {
this.result = checkNotNull(result, "result");
}

@Override
public PickResult pickSubchannel(PickSubchannelArgs args) {
return result;
}

@Override
public String toString() {
return MoreObjects.toStringHelper(Picker.class).add("result", result).toString();
}
}

/** Picker that requests connection during the first pick, and returns noResult. */
private final class RequestConnectionPicker extends SubchannelPicker {
private final AtomicBoolean connectionRequested = new AtomicBoolean(false);
Expand Down
11 changes: 5 additions & 6 deletions core/src/test/java/io/grpc/internal/PickFirstLoadBalancerTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -219,7 +219,7 @@ public void refreshNameResolutionAfterSubchannelConnectionBroken() {
inOrder.verify(mockSubchannel).start(stateListenerCaptor.capture());
SubchannelStateListener stateListener = stateListenerCaptor.getValue();
inOrder.verify(mockHelper).updateBalancingState(eq(CONNECTING), pickerCaptor.capture());
assertSame(mockSubchannel, pickerCaptor.getValue().pickSubchannel(mockArgs).getSubchannel());
assertThat(pickerCaptor.getValue().pickSubchannel(mockArgs).hasResult()).isFalse();
inOrder.verify(mockSubchannel).requestConnection();

stateListener.onSubchannelState(ConnectivityStateInfo.forNonError(CONNECTING));
Expand Down Expand Up @@ -278,7 +278,7 @@ public void pickAfterResolvedAndChanged() throws Exception {
assertThat(args.getAddresses()).isEqualTo(servers);
inOrder.verify(mockHelper).updateBalancingState(eq(CONNECTING), pickerCaptor.capture());
verify(mockSubchannel).requestConnection();
assertEquals(mockSubchannel, pickerCaptor.getValue().pickSubchannel(mockArgs).getSubchannel());
assertThat(pickerCaptor.getValue().pickSubchannel(mockArgs).hasResult()).isFalse();

loadBalancer.acceptResolvedAddresses(
ResolvedAddresses.newBuilder().setAddresses(newServers).setAttributes(affinity).build());
Expand All @@ -300,7 +300,7 @@ public void pickAfterStateChangeAfterResolution() throws Exception {
verify(mockSubchannel).start(stateListenerCaptor.capture());
SubchannelStateListener stateListener = stateListenerCaptor.getValue();
verify(mockHelper).updateBalancingState(eq(CONNECTING), pickerCaptor.capture());
Subchannel subchannel = pickerCaptor.getValue().pickSubchannel(mockArgs).getSubchannel();
assertThat(pickerCaptor.getValue().pickSubchannel(mockArgs).hasResult()).isFalse();
reset(mockHelper);
when(mockHelper.getSynchronizationContext()).thenReturn(syncContext);

Expand All @@ -317,7 +317,7 @@ public void pickAfterStateChangeAfterResolution() throws Exception {

stateListener.onSubchannelState(ConnectivityStateInfo.forNonError(READY));
inOrder.verify(mockHelper).updateBalancingState(eq(READY), pickerCaptor.capture());
assertEquals(subchannel, pickerCaptor.getValue().pickSubchannel(mockArgs).getSubchannel());
assertEquals(mockSubchannel, pickerCaptor.getValue().pickSubchannel(mockArgs).getSubchannel());

verify(mockHelper, atLeast(0)).getSynchronizationContext(); // Don't care
verifyNoMoreInteractions(mockHelper);
Expand Down Expand Up @@ -405,8 +405,7 @@ public void nameResolutionSuccessAfterError() throws Exception {
inOrder.verify(mockHelper).updateBalancingState(eq(CONNECTING), pickerCaptor.capture());
verify(mockSubchannel).requestConnection();

assertEquals(mockSubchannel, pickerCaptor.getValue().pickSubchannel(mockArgs)
.getSubchannel());
assertThat(pickerCaptor.getValue().pickSubchannel(mockArgs).hasResult()).isFalse();

assertEquals(pickerCaptor.getValue().pickSubchannel(mockArgs),
pickerCaptor.getValue().pickSubchannel(mockArgs));
Expand Down
Loading