Skip to content

core: Remove RetryingNR.RESOLUTION_RESULT_LISTENER_KEY #12150

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

Merged
merged 1 commit into from
Jun 13, 2025
Merged

core: Remove RetryingNR.RESOLUTION_RESULT_LISTENER_KEY #12150

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
14 changes: 1 addition & 13 deletions core/src/main/java/io/grpc/internal/ManagedChannelImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -94,7 +94,6 @@
import io.grpc.internal.ManagedChannelServiceConfig.ServiceConfigConvertedSelector;
import io.grpc.internal.RetriableStream.ChannelBufferMeter;
import io.grpc.internal.RetriableStream.Throttle;
import io.grpc.internal.RetryingNameResolver.ResolutionResultListener;
import java.net.URI;
import java.util.ArrayList;
import java.util.Collection;
Expand Down Expand Up @@ -1653,18 +1652,7 @@

@Override
public void onResult(final ResolutionResult resolutionResult) {
final class NamesResolved implements Runnable {

@Override
public void run() {
Status status = onResult2(resolutionResult);
ResolutionResultListener resolutionResultListener = resolutionResult.getAttributes()
.get(RetryingNameResolver.RESOLUTION_RESULT_LISTENER_KEY);
resolutionResultListener.resolutionAttempted(status);
}
}

syncContext.execute(new NamesResolved());
syncContext.execute(() -> onResult2(resolutionResult));

Check warning on line 1655 in core/src/main/java/io/grpc/internal/ManagedChannelImpl.java

View check run for this annotation

Codecov / codecov/patch

core/src/main/java/io/grpc/internal/ManagedChannelImpl.java#L1655 

Added line #L1655 was not covered by tests
}

@SuppressWarnings("ReferenceEquality")
Expand Down
32 changes: 1 addition & 31 deletions core/src/main/java/io/grpc/internal/RetryingNameResolver.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,6 @@
package io.grpc.internal;

import com.google.common.annotations.VisibleForTesting;
import io.grpc.Attributes;
import io.grpc.NameResolver;
import io.grpc.Status;
import io.grpc.SynchronizationContext;
Expand All @@ -34,9 +33,6 @@ final class RetryingNameResolver extends ForwardingNameResolver {
private final RetryScheduler retryScheduler;
private final SynchronizationContext syncContext;

static final Attributes.Key<ResolutionResultListener> RESOLUTION_RESULT_LISTENER_KEY
= Attributes.Key.create(
"io.grpc.internal.RetryingNameResolver.RESOLUTION_RESULT_LISTENER_KEY");

/**
* Creates a new {@link RetryingNameResolver}.
Expand Down Expand Up @@ -88,18 +84,7 @@ private class RetryingListener extends Listener2 {

@Override
public void onResult(ResolutionResult resolutionResult) {
// If the resolution result listener is already an attribute it indicates that a name resolver
// has already been wrapped with this class. This indicates a misconfiguration.
if (resolutionResult.getAttributes().get(RESOLUTION_RESULT_LISTENER_KEY) != null) {
throw new IllegalStateException(
"RetryingNameResolver can only be used once to wrap a NameResolver");
}

// To have retry behavior for name resolvers that haven't migrated to onResult2.
delegateListener.onResult(resolutionResult.toBuilder().setAttributes(
resolutionResult.getAttributes().toBuilder()
.set(RESOLUTION_RESULT_LISTENER_KEY, new ResolutionResultListener()).build())
.build());
syncContext.execute(() -> onResult2(resolutionResult));
}

@Override
Expand All @@ -119,19 +104,4 @@ public void onError(Status error) {
syncContext.execute(() -> retryScheduler.schedule(new DelayedNameResolverRefresh()));
}
}

/**
* Simple callback class to store in {@link ResolutionResult} attributes so that
* ManagedChannel can indicate if the resolved addresses were accepted. Temporary until
* the Listener2.onResult() API can be changed to return a boolean for this purpose.
*/
class ResolutionResultListener {
public void resolutionAttempted(Status successStatus) {
if (successStatus.isOk()) {
retryScheduler.reset();
} else {
retryScheduler.schedule(new DelayedNameResolverRefresh());
}
}
}
}
4 changes: 0 additions & 4 deletions core/src/test/java/io/grpc/internal/ManagedChannelImplTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3844,8 +3844,6 @@ public double nextDouble() {
verify(mockLoadBalancer).acceptResolvedAddresses(resolvedAddressCaptor.capture());
ResolvedAddresses resolvedAddresses = resolvedAddressCaptor.getValue();
assertThat(resolvedAddresses.getAddresses()).isEqualTo(nameResolverFactory.servers);
assertThat(resolvedAddresses.getAttributes()
.get(RetryingNameResolver.RESOLUTION_RESULT_LISTENER_KEY)).isNotNull();

// simulating request connection and then transport ready after resolved address
Subchannel subchannel =
Expand Down Expand Up @@ -3951,8 +3949,6 @@ public void hedgingScheduledThenChannelShutdown_hedgeShouldStillHappen_newCallSh
verify(mockLoadBalancer).acceptResolvedAddresses(resolvedAddressCaptor.capture());
ResolvedAddresses resolvedAddresses = resolvedAddressCaptor.getValue();
assertThat(resolvedAddresses.getAddresses()).isEqualTo(nameResolverFactory.servers);
assertThat(resolvedAddresses.getAttributes()
.get(RetryingNameResolver.RESOLUTION_RESULT_LISTENER_KEY)).isNotNull();

// simulating request connection and then transport ready after resolved address
Subchannel subchannel =
Expand Down
43 changes: 4 additions & 39 deletions core/src/test/java/io/grpc/internal/RetryingNameResolverTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,6 @@
package io.grpc.internal;

import static com.google.common.truth.Truth.assertThat;
import static org.junit.Assert.fail;
import static org.mockito.ArgumentMatchers.isA;
import static org.mockito.Mockito.mock;
import static org.mockito.Mockito.verify;
Expand All @@ -28,7 +27,6 @@
import io.grpc.NameResolver.ResolutionResult;
import io.grpc.Status;
import io.grpc.SynchronizationContext;
import io.grpc.internal.RetryingNameResolver.ResolutionResultListener;
import java.lang.Thread.UncaughtExceptionHandler;
import org.junit.Before;
import org.junit.Rule;
Expand Down Expand Up @@ -58,8 +56,6 @@ public class RetryingNameResolverTest {
private RetryScheduler mockRetryScheduler;
@Captor
private ArgumentCaptor<Listener2> listenerCaptor;
@Captor
private ArgumentCaptor<ResolutionResult> onResultCaptor;
private final SynchronizationContext syncContext = new SynchronizationContext(
mock(UncaughtExceptionHandler.class));

Expand All @@ -77,21 +73,14 @@ public void startAndShutdown() {
retryingNameResolver.shutdown();
}

// Make sure the ResolutionResultListener callback is added to the ResolutionResult attributes,
// and the retry scheduler is reset since the name resolution was successful.
@Test
public void onResult_success() {
when(mockListener.onResult2(isA(ResolutionResult.class))).thenReturn(Status.OK);
retryingNameResolver.start(mockListener);
verify(mockNameResolver).start(listenerCaptor.capture());

listenerCaptor.getValue().onResult(ResolutionResult.newBuilder().build());
verify(mockListener).onResult(onResultCaptor.capture());
ResolutionResultListener resolutionResultListener = onResultCaptor.getValue()
.getAttributes()
.get(RetryingNameResolver.RESOLUTION_RESULT_LISTENER_KEY);
assertThat(resolutionResultListener).isNotNull();

resolutionResultListener.resolutionAttempted(Status.OK);
verify(mockRetryScheduler).reset();
}

Expand All @@ -107,21 +96,15 @@ public void onResult2_sucesss() {
verify(mockRetryScheduler).reset();
}

// Make sure the ResolutionResultListener callback is added to the ResolutionResult attributes,
// and that a retry gets scheduled when the resolution results are rejected.
// Make sure that a retry gets scheduled when the resolution results are rejected.
@Test
public void onResult_failure() {
when(mockListener.onResult2(isA(ResolutionResult.class))).thenReturn(Status.UNAVAILABLE);
retryingNameResolver.start(mockListener);
verify(mockNameResolver).start(listenerCaptor.capture());

listenerCaptor.getValue().onResult(ResolutionResult.newBuilder().build());
verify(mockListener).onResult(onResultCaptor.capture());
ResolutionResultListener resolutionResultListener = onResultCaptor.getValue()
.getAttributes()
.get(RetryingNameResolver.RESOLUTION_RESULT_LISTENER_KEY);
assertThat(resolutionResultListener).isNotNull();

resolutionResultListener.resolutionAttempted(Status.UNAVAILABLE);
verify(mockRetryScheduler).schedule(isA(Runnable.class));
}

Expand All @@ -138,24 +121,6 @@ public void onResult2_failure() {
verify(mockRetryScheduler).schedule(isA(Runnable.class));
}

// Wrapping a NameResolver more than once is a misconfiguration.
@Test
public void onResult_failure_doubleWrapped() {
NameResolver doubleWrappedResolver = new RetryingNameResolver(retryingNameResolver,
mockRetryScheduler, syncContext);

doubleWrappedResolver.start(mockListener);
verify(mockNameResolver).start(listenerCaptor.capture());

try {
listenerCaptor.getValue().onResult(ResolutionResult.newBuilder().build());
} catch (IllegalStateException e) {
assertThat(e).hasMessageThat().contains("can only be used once");
return;
}
fail("An exception should have been thrown for a double wrapped NAmeResolver");
}

// A retry should get scheduled when name resolution fails.
@Test
public void onError() {
Expand All @@ -165,4 +130,4 @@ public void onError() {
verify(mockListener).onError(Status.DEADLINE_EXCEEDED);
verify(mockRetryScheduler).schedule(isA(Runnable.class));
}
}
}
Loading