Skip to content

Commit

Permalink
api: use <scheme,provider> map in nameResoverRegistry (#8323)
Browse files Browse the repository at this point in the history 
An improvement that makes name resolver provider scheme matching more explicit in name resolver registry.
  • Loading branch information
@YifeiZhuang
YifeiZhuang committed Jul 21, 2021
1 parent a282019 commit 4c1272f
Show file tree
Hide file tree
Showing 4 changed files with 118 additions and 45 deletions.
14 changes: 14 additions & 0 deletions api/src/main/java/io/grpc/NameResolverProvider.java
View file
Expand Up @@ -16,6 +16,8 @@

package io.grpc;

import io.grpc.NameResolver.Factory;

/**
* Provider of name resolvers for name agnostic consumption.
*
Expand Down Expand Up @@ -48,4 +50,16 @@ public abstract class NameResolverProvider extends NameResolver.Factory {
* @since 1.0.0
*/
protected abstract int priority();

/**
* Returns the scheme associated with the provider. The provider normally should only create a
* {@link NameResolver} when target URI scheme matches the provider scheme. It temporarily
* delegates to {@link Factory#getDefaultScheme()} before {@link NameResolver.Factory} is
* deprecated in https://github.com/grpc/grpc-java/issues/7133.
*
* @since 1.40.0
* */
protected String getScheme() {
return getDefaultScheme();
}
}
59 changes: 33 additions & 26 deletions api/src/main/java/io/grpc/NameResolverRegistry.java
View file
Expand Up @@ -19,12 +19,14 @@
import static com.google.common.base.Preconditions.checkArgument;

import com.google.common.annotations.VisibleForTesting;
import com.google.common.collect.ImmutableMap;
import java.net.URI;
import java.util.ArrayList;
import java.util.Collections;
import java.util.Comparator;
import java.util.HashMap;
import java.util.LinkedHashSet;
import java.util.List;
import java.util.Map;
import java.util.logging.Level;
import java.util.logging.Logger;
import javax.annotation.Nullable;
Expand All @@ -44,12 +46,17 @@ public final class NameResolverRegistry {
private static NameResolverRegistry instance;

private final NameResolver.Factory factory = new NameResolverFactory();
private static final String UNKNOWN_SCHEME = "unknown";
@GuardedBy("this")
private String defaultScheme = UNKNOWN_SCHEME;

@GuardedBy("this")
private final LinkedHashSet<NameResolverProvider> allProviders = new LinkedHashSet<>();
/** Immutable, sorted version of {@code allProviders}. Is replaced instead of mutating. */
/** Generated from {@code allProviders}. Is mapping from scheme key to the highest priority
* {@link NameResolverProvider}. Is replaced instead of mutating. */
@GuardedBy("this")
private List<NameResolverProvider> effectiveProviders = Collections.emptyList();
private ImmutableMap<String, NameResolverProvider> effectiveProviders = ImmutableMap.of();


/**
* Register a provider.
Expand Down Expand Up @@ -81,16 +88,23 @@ public synchronized void deregister(NameResolverProvider provider) {
}

private synchronized void refreshProviders() {
List<NameResolverProvider> providers = new ArrayList<>(allProviders);
// Sort descending based on priority.
// sort() must be stable, as we prefer first-registered providers
Collections.sort(providers, Collections.reverseOrder(new Comparator<NameResolverProvider>() {
@Override
public int compare(NameResolverProvider o1, NameResolverProvider o2) {
return o1.priority() - o2.priority();
Map<String, NameResolverProvider> refreshedProviders = new HashMap<>();
int maxPriority = Integer.MIN_VALUE;
String refreshedDefaultScheme = UNKNOWN_SCHEME;
// We prefer first-registered providers
for (NameResolverProvider provider : allProviders) {
String scheme = provider.getScheme();
NameResolverProvider existing = refreshedProviders.get(scheme);
if (existing == null || existing.priority() < provider.priority()) {
refreshedProviders.put(scheme, provider);
}
if (maxPriority < provider.priority()) {
maxPriority = provider.priority();
refreshedDefaultScheme = provider.getScheme();
}
}));
effectiveProviders = Collections.unmodifiableList(providers);
}
effectiveProviders = ImmutableMap.copyOf(refreshedProviders);
defaultScheme = refreshedDefaultScheme;
}

/**
Expand Down Expand Up @@ -120,10 +134,11 @@ public static synchronized NameResolverRegistry getDefaultRegistry() {
}

/**
* Returns effective providers, in priority order.
* Returns effective providers map from scheme to the highest priority NameResolverProvider of
* that scheme.
*/
@VisibleForTesting
synchronized List<NameResolverProvider> providers() {
synchronized Map<String, NameResolverProvider> providers() {
return effectiveProviders;
}

Expand All @@ -149,23 +164,15 @@ private final class NameResolverFactory extends NameResolver.Factory {
@Override
@Nullable
public NameResolver newNameResolver(URI targetUri, NameResolver.Args args) {
List<NameResolverProvider> providers = providers();
for (NameResolverProvider provider : providers) {
NameResolver resolver = provider.newNameResolver(targetUri, args);
if (resolver != null) {
return resolver;
}
}
return null;
NameResolverProvider provider = providers().get(targetUri.getScheme());
return provider == null ? null : provider.newNameResolver(targetUri, args);
}

@Override
public String getDefaultScheme() {
List<NameResolverProvider> providers = providers();
if (providers.isEmpty()) {
return "unknown";
synchronized (NameResolverRegistry.this) {
return defaultScheme;
}
return providers.get(0).getDefaultScheme();
}
}

Expand Down
82 changes: 67 additions & 15 deletions api/src/test/java/io/grpc/NameResolverRegistryTest.java
View file
Expand Up @@ -25,6 +25,7 @@
import java.lang.Thread.UncaughtExceptionHandler;
import java.net.URI;
import java.util.List;
import java.util.Map;
import org.junit.Test;
import org.junit.runner.RunWith;
import org.junit.runners.JUnit4;
Expand Down Expand Up @@ -59,12 +60,16 @@ public void deregister() {
NameResolverProvider p1 = new BaseProvider(true, 5);
NameResolverProvider p2 = new BaseProvider(true, 5);
NameResolverProvider p3 = new BaseProvider(true, 5);
String sameScheme = p1.getDefaultScheme();
reg.register(p1);
reg.register(p2);
reg.register(p3);
assertThat(reg.providers()).containsExactly(p1, p2, p3).inOrder();
assertThat(reg.providers().get(sameScheme)).isSameInstanceAs(p1);
reg.deregister(p2);
assertThat(reg.providers()).containsExactly(p1, p3).inOrder();
assertThat(reg.providers().get(sameScheme)).isSameInstanceAs(p1);
reg.deregister(p1);
assertThat(reg.providers().get(sameScheme)).isSameInstanceAs(p3);

}

@Test
Expand All @@ -75,12 +80,13 @@ public void provider_sorted() {
NameResolverProvider p3 = new BaseProvider(true, 8);
NameResolverProvider p4 = new BaseProvider(true, 3);
NameResolverProvider p5 = new BaseProvider(true, 8);
String sameScheme = p1.getDefaultScheme();
reg.register(p1);
reg.register(p2);
reg.register(p3);
reg.register(p4);
reg.register(p5);
assertThat(reg.providers()).containsExactly(p3, p5, p1, p2, p4).inOrder();
assertThat(reg.providers().get(sameScheme)).isSameInstanceAs(p3);
}

@Test
Expand All @@ -93,7 +99,7 @@ public void getDefaultScheme_noProvider() {
public void newNameResolver_providerReturnsNull() {
NameResolverRegistry registry = new NameResolverRegistry();
registry.register(
new BaseProvider(true, 5) {
new BaseProvider(true, 5, "noScheme") {
@Override
public NameResolver newNameResolver(URI passedUri, NameResolver.Args passedArgs) {
assertThat(passedUri).isSameInstanceAs(uri);
Expand All @@ -102,12 +108,13 @@ public NameResolver newNameResolver(URI passedUri, NameResolver.Args passedArgs)
}
});
assertThat(registry.asFactory().newNameResolver(uri, args)).isNull();
assertThat(registry.asFactory().getDefaultScheme()).isEqualTo("noScheme");
}

@Test
public void newNameResolver_providerReturnsNonNull() {
NameResolverRegistry registry = new NameResolverRegistry();
registry.register(new BaseProvider(true, 5) {
registry.register(new BaseProvider(true, 5, uri.getScheme()) {
@Override
public NameResolver newNameResolver(URI passedUri, NameResolver.Args passedArgs) {
return null;
Expand All @@ -127,38 +134,75 @@ public NameResolver newNameResolver(URI passedUri, NameResolver.Args passedArgs)
}
};
registry.register(
new BaseProvider(true, 4) {
new BaseProvider(true, 4, uri.getScheme()) {
@Override
public NameResolver newNameResolver(URI passedUri, NameResolver.Args passedArgs) {
return nr;
}
});
registry.register(
new BaseProvider(true, 3) {
new BaseProvider(true, 3, uri.getScheme()) {
@Override
public NameResolver newNameResolver(URI passedUri, NameResolver.Args passedArgs) {
fail("Should not be called");
throw new AssertionError();
}
});
assertThat(registry.asFactory().newNameResolver(uri, args)).isSameInstanceAs(nr);
assertThat(registry.asFactory().newNameResolver(uri, args)).isNull();
assertThat(registry.asFactory().getDefaultScheme()).isEqualTo(uri.getScheme());
}

@Test
public void newNameResolver_multipleScheme() {
NameResolverRegistry registry = new NameResolverRegistry();
registry.register(new BaseProvider(true, 5, uri.getScheme()) {
@Override
public NameResolver newNameResolver(URI passedUri, NameResolver.Args passedArgs) {
return null;
}
});
final NameResolver nr = new NameResolver() {
@Override public String getServiceAuthority() {
throw new UnsupportedOperationException();
}

@Override public void start(Listener2 listener) {
throw new UnsupportedOperationException();
}

@Override public void shutdown() {
throw new UnsupportedOperationException();
}
};
registry.register(
new BaseProvider(true, 4, "other") {
@Override
public NameResolver newNameResolver(URI passedUri, NameResolver.Args passedArgs) {
return nr;
}
});

assertThat(registry.asFactory().newNameResolver(uri, args)).isNull();
assertThat(registry.asFactory().newNameResolver(URI.create("other:///0.0.0.0:80"), args))
.isSameInstanceAs(nr);
assertThat(registry.asFactory().getDefaultScheme()).isEqualTo("dns");
}

@Test
public void newNameResolver_noProvider() {
NameResolver.Factory factory = new NameResolverRegistry().asFactory();
assertThat(factory.newNameResolver(uri, args)).isNull();
assertThat(factory.getDefaultScheme()).isEqualTo("unknown");
}

@Test
public void baseProviders() {
List<NameResolverProvider> providers = NameResolverRegistry.getDefaultRegistry().providers();
assertThat(providers).hasSize(2);
// 2 name resolvers from grpclb and core, ordered with decreasing priorities.
assertThat(providers.get(0).getClass().getName())
Map<String, NameResolverProvider> providers =
NameResolverRegistry.getDefaultRegistry().providers();
assertThat(providers).hasSize(1);
// 2 name resolvers from grpclb and core, higher priority one is returned.
assertThat(providers.get("dns").getClass().getName())
.isEqualTo("io.grpc.grpclb.SecretGrpclbNameResolverProvider$Provider");
assertThat(providers.get(1).getClass().getName())
.isEqualTo("io.grpc.internal.DnsNameResolverProvider");
assertThat(NameResolverRegistry.getDefaultRegistry().asFactory().getDefaultScheme())
.isEqualTo("dns");
}
Expand All @@ -184,10 +228,18 @@ NameResolverProvider.class, getClass().getClassLoader())) {
private static class BaseProvider extends NameResolverProvider {
private final boolean isAvailable;
private final int priority;
private final String scheme;

public BaseProvider(boolean isAvailable, int priority) {
this.isAvailable = isAvailable;
this.priority = priority;
this.scheme = null;
}

public BaseProvider(boolean isAvailable, int priority, String scheme) {
this.isAvailable = isAvailable;
this.priority = priority;
this.scheme = scheme;
}

@Override
Expand All @@ -207,7 +259,7 @@ public NameResolver newNameResolver(URI targetUri, NameResolver.Args args) {

@Override
public String getDefaultScheme() {
return "scheme" + getClass().getSimpleName();
return scheme == null ? "scheme" + getClass().getSimpleName() : scheme;
}
}
}
8 changes: 4 additions & 4 deletions xds/src/test/java/io/grpc/xds/XdsSdsClientServerTest.java
View file
Expand Up @@ -88,8 +88,11 @@ public class XdsSdsClientServerTest {
private TlsContextManagerImpl tlsContextManagerForServer;

@Before
public void setUp() throws IOException {
public void setUp() throws Exception {
port = XdsServerTestHelper.findFreePort();
URI expectedUri = new URI("sdstest://localhost:" + port);
fakeNameResolverFactory = new FakeNameResolverFactory.Builder(expectedUri).build();
NameResolverRegistry.getDefaultRegistry().register(fakeNameResolverFactory);
}

@After
Expand Down Expand Up @@ -410,9 +413,6 @@ static EnvoyServerProtoData.Listener buildListener(
private SimpleServiceGrpc.SimpleServiceBlockingStub getBlockingStub(
final UpstreamTlsContext upstreamTlsContext, String overrideAuthority)
throws URISyntaxException {
URI expectedUri = new URI("sdstest://localhost:" + port);
fakeNameResolverFactory = new FakeNameResolverFactory.Builder(expectedUri).build();
NameResolverRegistry.getDefaultRegistry().register(fakeNameResolverFactory);
ManagedChannelBuilder<?> channelBuilder =
Grpc.newChannelBuilder(
"sdstest://localhost:" + port,
Expand Down

0 comments on commit 4c1272f

Please sign in to comment.