Skip to content

Commit

Permalink
Listener.onError is only called when name resolution error. Service c…
Browse files Browse the repository at this point in the history 
…onfig parse error doesn't trigger onError except when no default config on the first name resolution with service config parse error.
  • Loading branch information
@creamsoup
creamsoup committed Nov 15, 2019
1 parent a227050 commit 21c96a4
Show file tree
Hide file tree
Showing 5 changed files with 61 additions and 64 deletions.
22 changes: 2 additions & 20 deletions api/src/main/java/io/grpc/NameResolver.java
View file
Expand Up @@ -18,7 +18,6 @@

import static com.google.common.base.Preconditions.checkArgument;
import static com.google.common.base.Preconditions.checkNotNull;
import static com.google.common.base.Preconditions.checkState;

import com.google.common.base.MoreObjects;
import com.google.common.base.Objects;
Expand Down Expand Up @@ -331,31 +330,14 @@ public final void onAddresses(
public abstract void onResult(ResolutionResult resolutionResult);

/**
* Handles an error from the resolver. The listener is responsible for eventually invoking
* {@link NameResolver#refresh()} to re-attempt resolution.
* Handles a name resolving error from the resolver. The listener is responsible for eventually
* invoking {@link NameResolver#refresh()} to re-attempt resolution.
*
* @param error a non-OK status
* @since 1.21.0
*/
@Override
public abstract void onError(Status error);

/**
* Handles an error from the resolver. If the error is not recoverable, it calls {@link
* #onError(Status)}.
*
* @param resolutionResult the resolved server addresses, attributes, and Service Config.
* @since 1.26.0
*/
public void handleError(ResolutionResult resolutionResult) {
checkNotNull(
resolutionResult.getServiceConfig(),
"Resolution result should have a ServiceConfig with error");
Status error = resolutionResult.getServiceConfig().getError();
checkState(error != null && !error.isOk(), "ServiceConfig has no error");

onError(error);
}
}

/**
Expand Down
11 changes: 5 additions & 6 deletions core/src/main/java/io/grpc/internal/DnsNameResolver.java
View file
Expand Up @@ -308,23 +308,22 @@ public void run() {
ConfigOrError serviceConfig =
parseServiceConfig(resolutionResults.txtRecords, random, getLocalHostname());
if (serviceConfig != null) {
if (serviceConfig.getError() != null) {
savedListener.handleError(resultBuilder.setServiceConfig(serviceConfig).build());
return;
} else {
resultBuilder.setServiceConfig(serviceConfig);
if (serviceConfig.getError() == null) {
@SuppressWarnings("unchecked")
Map<String, ?> config = (Map<String, ?>) serviceConfig.getConfig();
resultBuilder
.setAttributes(
Attributes.newBuilder()
.set(GrpcAttributes.NAME_RESOLVER_SERVICE_CONFIG, config)
.build())
.setServiceConfig(serviceConfig);
.build());
}

}
} else {
logger.log(Level.FINE, "No TXT records found for {0}", new Object[]{host});
}

savedListener.onResult(resultBuilder.build());
}
}
Expand Down
52 changes: 25 additions & 27 deletions core/src/main/java/io/grpc/internal/ManagedChannelImpl.java
View file
Expand Up @@ -130,7 +130,7 @@ final class ManagedChannelImpl extends ManagedChannel implements
static final Status SUBCHANNEL_SHUTDOWN_STATUS =
Status.UNAVAILABLE.withDescription("Subchannel shutdown invoked");

private static final Map<String, Object> EMPTY_SERVICE_CONFIG = Collections.emptyMap();
private static final Map<String, ?> EMPTY_SERVICE_CONFIG = Collections.emptyMap();

private final InternalLogId logId;
private final String target;
Expand Down Expand Up @@ -1302,24 +1302,34 @@ private final class NameResolverListener extends NameResolver.Listener2 {
public void onResult(final ResolutionResult resolutionResult) {
final class NamesResolved implements Runnable {

@SuppressWarnings("ReferenceEquality")
@SuppressWarnings({"ReferenceEquality", "unchecked"})
@Override
public void run() {
List<EquivalentAddressGroup> servers = resolutionResult.getAddresses();
Attributes attrs = resolutionResult.getAttributes();
channelLogger.log(
ChannelLogLevel.DEBUG, "Resolved address: {0}, config={1}", servers, attrs);

boolean nrReplied = haveBackends == null;

if (haveBackends == null || !haveBackends) {
channelLogger.log(ChannelLogLevel.INFO, "Address resolved: {0}", servers);
haveBackends = true;
}

nameResolverBackoffPolicy = null;
ConfigOrError configOrError = resolutionResult.getServiceConfig();
Map<String, ?> serviceConfig = null;
if (configOrError != null) {
if (configOrError.getConfig() != null) {
serviceConfig = (Map<String, ?>) configOrError.getConfig();
} else if (nrReplied || defaultServiceConfig == null) {
// First DNS lookup has invalid service config, and cannot fall back to default(=null)
onError(configOrError.getError());
return;
}
}

// Assuming no error in config resolution for now.
final Map<String, ?> serviceConfig =
attrs.get(GrpcAttributes.NAME_RESOLVER_SERVICE_CONFIG);
Map<String, ?> effectiveServiceConfig;
if (!lookUpServiceConfig) {
if (serviceConfig != null) {
Expand All @@ -1333,19 +1343,17 @@ public void run() {
// Otherwise, try to use the default config if available
if (serviceConfig != null) {
effectiveServiceConfig = serviceConfig;
} else if (defaultServiceConfig != null) {
effectiveServiceConfig = defaultServiceConfig;
channelLogger.log(
ChannelLogLevel.INFO,
"Received no service config, using default service config");
} else {
effectiveServiceConfig = defaultServiceConfig != null
? defaultServiceConfig : EMPTY_SERVICE_CONFIG;
if (defaultServiceConfig != null) {
channelLogger.log(
ChannelLogLevel.INFO,
"Received no service config, using default service config");
} else {
channelLogger.log(
ChannelLogLevel.INFO,
"Received no service config and default service config is not provided. "
+ "Falling back to empty service config");
}
effectiveServiceConfig = EMPTY_SERVICE_CONFIG;
channelLogger.log(
ChannelLogLevel.INFO,
"Received no service config and default service config is not provided. "
+ "Falling back to empty service config");
}

// FIXME(notcarl): reference equality is not right (although not harmful) right now.
Expand Down Expand Up @@ -1388,16 +1396,6 @@ public void run() {
syncContext.execute(new NamesResolved());
}

@Override
public void handleError(ResolutionResult resolutionResult) {
boolean nrReplied = haveBackends == null;
if (!nrReplied) {
onResult(
ResolutionResult.newBuilder().setAddresses(resolutionResult.getAddresses()).build());
}
super.handleError(resolutionResult);
}

@Override
public void onError(final Status error) {
checkArgument(!error.isOk(), "the error status must not be OK");
Expand Down
28 changes: 20 additions & 8 deletions core/src/test/java/io/grpc/internal/ManagedChannelImplTest.java
View file
Expand Up @@ -2812,6 +2812,7 @@ public void channelTracing_serviceConfigChange() throws Exception {
new EquivalentAddressGroup(
Arrays.asList(new SocketAddress() {}, new SocketAddress() {}))))
.setAttributes(attributes)
.setServiceConfig(ConfigOrError.fromConfig(new HashMap<String, Object>()))
.build();
nameResolverFactory.resolvers.get(0).listener.onResult(resolutionResult1);
assertThat(getStats(channel).channelTrace.events).hasSize(prevSize + 1);
Expand All @@ -2828,9 +2829,10 @@ public void channelTracing_serviceConfigChange() throws Exception {
new EquivalentAddressGroup(
Arrays.asList(new SocketAddress() {}, new SocketAddress() {}))))
.setAttributes(attributes)
.setServiceConfig(ConfigOrError.fromConfig(new HashMap<String, Object>()))
.build();
nameResolverFactory.resolvers.get(0).listener.onResult(resolutionResult2);
assertThat(getStats(channel).channelTrace.events).hasSize(prevSize);
assertThat(getStats(channel).channelTrace.events).hasSize(prevSize + 1);

prevSize = getStats(channel).channelTrace.events.size();
Map<String, Object> serviceConfig = new HashMap<>();
Expand All @@ -2845,6 +2847,7 @@ public void channelTracing_serviceConfigChange() throws Exception {
new EquivalentAddressGroup(
Arrays.asList(new SocketAddress() {}, new SocketAddress() {}))))
.setAttributes(attributes)
.setServiceConfig(ConfigOrError.fromConfig(new HashMap<String, Object>()))
.build();
nameResolverFactory.resolvers.get(0).listener.onResult(resolutionResult3);
assertThat(getStats(channel).channelTrace.events).hasSize(prevSize + 1);
Expand Down Expand Up @@ -3408,15 +3411,17 @@ public String getServiceAuthority() {
@Override
public void start(Listener2 listener) {
this.listener = listener;
ImmutableMap<String, Object> serviceConfig =
ImmutableMap.<String, Object>of("loadBalancingPolicy", "kaboom");
listener.onResult(
ResolutionResult.newBuilder()
.setAddresses(addresses)
.setAttributes(
Attributes.newBuilder()
.set(
GrpcAttributes.NAME_RESOLVER_SERVICE_CONFIG,
ImmutableMap.<String, Object>of("loadBalancingPolicy", "kaboom"))
GrpcAttributes.NAME_RESOLVER_SERVICE_CONFIG, serviceConfig)
.build())
.setServiceConfig(ConfigOrError.fromConfig(serviceConfig))
.build());
}

Expand Down Expand Up @@ -3469,15 +3474,17 @@ protected ClientTransportFactory buildTransportFactory() {

// ok the service config is bad, let's fix it.

ImmutableMap<String, Object> serviceConfig =
ImmutableMap.<String, Object>of("loadBalancingPolicy", "round_robin");
factory.resolver.listener.onResult(
ResolutionResult.newBuilder()
.setAddresses(addresses)
.setAttributes(
Attributes.newBuilder()
.set(
GrpcAttributes.NAME_RESOLVER_SERVICE_CONFIG,
ImmutableMap.<String, Object>of("loadBalancingPolicy", "round_robin"))
GrpcAttributes.NAME_RESOLVER_SERVICE_CONFIG, serviceConfig)
.build())
.setServiceConfig(ConfigOrError.fromConfig(serviceConfig))
.build());

ClientCall<Void, Void> call2 = mychannel.newCall(
Expand Down Expand Up @@ -4056,11 +4063,16 @@ void resolved() {
listener.onError(error);
return;
}
listener.onResult(
Attributes attr = nextResolvedAttributes.get();
Map<String, ?> config = attr.get(GrpcAttributes.NAME_RESOLVER_SERVICE_CONFIG);
ResolutionResult.Builder builder =
ResolutionResult.newBuilder()
.setAddresses(servers)
.setAttributes(nextResolvedAttributes.get())
.build());
.setAttributes(attr);
if (config != null) {
builder.setServiceConfig(ConfigOrError.fromConfig(config));
}
listener.onResult(builder.build());
}

@Override public void shutdown() {
Expand Down
12 changes: 9 additions & 3 deletions core/src/test/java/io/grpc/internal/ServiceConfigErrorHandlingTest.java
View file
Expand Up @@ -48,6 +48,7 @@
import java.util.Arrays;
import java.util.Collections;
import java.util.List;
import java.util.Map;
import java.util.concurrent.Executor;
import java.util.concurrent.TimeUnit;
import java.util.concurrent.atomic.AtomicReference;
Expand Down Expand Up @@ -518,11 +519,16 @@ void resolved() {
listener.onError(error);
return;
}
listener.onResult(
Attributes attr = nextResolvedAttributes.get();
Map<String, ?> config = attr.get(GrpcAttributes.NAME_RESOLVER_SERVICE_CONFIG);
ResolutionResult.Builder builder =
ResolutionResult.newBuilder()
.setAddresses(servers)
.setAttributes(nextResolvedAttributes.get())
.build());
.setAttributes(attr);
if (config != null) {
builder.setServiceConfig(ConfigOrError.fromConfig(config));
}
listener.onResult(builder.build());
}

@Override public void shutdown() {
Expand Down

0 comments on commit 21c96a4

Please sign in to comment.