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

core: Apply RetryingNameResolver in ManagedChannelImpl #10371

Merged
merged 2 commits into from
Jul 12, 2023
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
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
33 changes: 20 additions & 13 deletions core/src/main/java/io/grpc/internal/DnsNameResolverProvider.java
Original file line number Diff line number Diff line change
Expand Up @@ -56,19 +56,26 @@ public NameResolver newNameResolver(URI targetUri, NameResolver.Args args) {
Preconditions.checkArgument(targetPath.startsWith("/"),
"the path component (%s) of the target (%s) must start with '/'", targetPath, targetUri);
String name = targetPath.substring(1);
return new RetryingNameResolver(
new DnsNameResolver(
targetUri.getAuthority(),
name,
args,
GrpcUtil.SHARED_CHANNEL_EXECUTOR,
Stopwatch.createUnstarted(),
IS_ANDROID),
new BackoffPolicyRetryScheduler(
new ExponentialBackoffPolicy.Provider(),
args.getScheduledExecutorService(),
args.getSynchronizationContext()),
args.getSynchronizationContext());
return new DnsNameResolver(
targetUri.getAuthority(),
name,
args,
GrpcUtil.SHARED_CHANNEL_EXECUTOR,
Stopwatch.createUnstarted(),
IS_ANDROID);
// return new RetryingNameResolver(
Copy link
Member

Choose a reason for hiding this comment

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

This is a bit strange. Remove it or add a comment?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That was an oversight, removed.

// new DnsNameResolver(
// targetUri.getAuthority(),
// name,
// args,
// GrpcUtil.SHARED_CHANNEL_EXECUTOR,
// Stopwatch.createUnstarted(),
// IS_ANDROID),
// new BackoffPolicyRetryScheduler(
// new ExponentialBackoffPolicy.Provider(),
// args.getScheduledExecutorService(),
// args.getSynchronizationContext()),
// args.getSynchronizationContext());
} else {
return null;
}
Expand Down
11 changes: 2 additions & 9 deletions core/src/main/java/io/grpc/internal/ManagedChannelImpl.java
Original file line number Diff line number Diff line change
Expand Up @@ -750,21 +750,14 @@ static NameResolver getNameResolver(
NameResolver.Factory nameResolverFactory, NameResolver.Args nameResolverArgs) {
NameResolver resolver = getNameResolver(target, nameResolverFactory, nameResolverArgs);

// If the nameResolver is not already a RetryingNameResolver, then wrap it with it.
// This helps guarantee that name resolution retry remains supported even as it has been
// removed from ManagedChannelImpl.
// We wrap the name resolver in a RetryingNameResolver to give it the ability to retry failures.
// TODO: After a transition period, all NameResolver implementations that need retry should use
// RetryingNameResolver directly and this step can be removed.
NameResolver usedNameResolver;
if (resolver instanceof RetryingNameResolver) {
usedNameResolver = resolver;
} else {
usedNameResolver = new RetryingNameResolver(resolver,
NameResolver usedNameResolver = new RetryingNameResolver(resolver,
new BackoffPolicyRetryScheduler(new ExponentialBackoffPolicy.Provider(),
nameResolverArgs.getScheduledExecutorService(),
nameResolverArgs.getSynchronizationContext()),
nameResolverArgs.getSynchronizationContext());
}

if (overrideAuthority == null) {
return usedNameResolver;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -61,9 +61,7 @@ public void isAvailable() {
@Test
public void newNameResolver() {
assertSame(DnsNameResolver.class,
((RetryingNameResolver) provider.newNameResolver(
URI.create("dns:///localhost:443"), args))
.getRetriedNameResolver().getClass());
provider.newNameResolver(URI.create("dns:///localhost:443"), args).getClass());
assertNull(
provider.newNameResolver(URI.create("notdns:///localhost:443"), args));
}
Expand Down
3 changes: 1 addition & 2 deletions core/src/test/java/io/grpc/internal/DnsNameResolverTest.java
Original file line number Diff line number Diff line change
Expand Up @@ -1293,8 +1293,7 @@ private void testInvalidUri(URI uri) {
}

private void testValidUri(URI uri, String exportedAuthority, int expectedPort) {
DnsNameResolver resolver = (DnsNameResolver) ((RetryingNameResolver) provider.newNameResolver(
uri, args)).getRetriedNameResolver();
DnsNameResolver resolver = (DnsNameResolver) provider.newNameResolver(uri, args);
assertNotNull(resolver);
assertEquals(expectedPort, resolver.getPort());
assertEquals(exportedAuthority, resolver.getServiceAuthority());
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -542,7 +542,7 @@ private static final class FakeNameResolverFactory extends NameResolver.Factory
final URI expectedUri;
final List<EquivalentAddressGroup> servers;
final boolean resolvedAtStart;
final ArrayList<RetryingNameResolver> resolvers = new ArrayList<>();
final ArrayList<FakeNameResolver> resolvers = new ArrayList<>();
final AtomicReference<Map<String, ?>> nextRawServiceConfig = new AtomicReference<>();
final AtomicReference<Attributes> nextAttributes = new AtomicReference<>(Attributes.EMPTY);

Expand All @@ -561,13 +561,7 @@ public NameResolver newNameResolver(final URI targetUri, NameResolver.Args args)
return null;
}
assertEquals(DEFAULT_PORT, args.getDefaultPort());
RetryingNameResolver resolver = new RetryingNameResolver(
new FakeNameResolver(args.getServiceConfigParser()),
new BackoffPolicyRetryScheduler(
new FakeBackoffPolicyProvider(),
args.getScheduledExecutorService(),
args.getSynchronizationContext()),
args.getSynchronizationContext());
FakeNameResolver resolver = new FakeNameResolver(args.getServiceConfigParser());
resolvers.add(resolver);
return resolver;
}
Expand All @@ -578,8 +572,8 @@ public String getDefaultScheme() {
}

void allResolved() {
for (RetryingNameResolver resolver : resolvers) {
((FakeNameResolver)resolver.getRetriedNameResolver()).resolved();
for (FakeNameResolver resolver : resolvers) {
resolver.resolved();
}
}

Expand Down