diff --git a/core/src/main/java/io/grpc/internal/DnsNameResolver.java b/core/src/main/java/io/grpc/internal/DnsNameResolver.java index dacbe291b6b..809b6f9ff53 100644 --- a/core/src/main/java/io/grpc/internal/DnsNameResolver.java +++ b/core/src/main/java/io/grpc/internal/DnsNameResolver.java @@ -25,7 +25,6 @@ import com.google.common.base.Stopwatch; import com.google.common.base.Verify; import com.google.common.base.VerifyException; -import io.grpc.Attributes; import io.grpc.EquivalentAddressGroup; import io.grpc.NameResolver; import io.grpc.ProxiedSocketAddress; @@ -262,22 +261,19 @@ private EquivalentAddressGroup detectProxy() throws IOException { /** * Main logic of name resolution. */ - protected InternalResolutionResult doResolve(boolean forceTxt) { - InternalResolutionResult result = new InternalResolutionResult(); + protected ResolutionResult doResolve() { + ResolutionResult.Builder resultBuilder = ResolutionResult.newBuilder(); try { - result.addresses = resolveAddresses(); + resultBuilder.setAddressesOrError(StatusOr.fromValue(resolveAddresses())); } catch (Exception e) { logger.log(Level.FINE, "Address resolution failure", e); - if (!forceTxt) { - result.error = - Status.UNAVAILABLE.withDescription("Unable to resolve host " + host).withCause(e); - return result; - } + resultBuilder.setAddressesOrError(StatusOr.fromStatus( + Status.UNAVAILABLE.withDescription("Unable to resolve host " + host).withCause(e))); } if (enableTxt) { - result.config = resolveServiceConfig(); + resultBuilder.setServiceConfig(resolveServiceConfig()); } - return result; + return resultBuilder.build(); } private final class Resolve implements Runnable { @@ -292,38 +288,22 @@ public void run() { if (logger.isLoggable(Level.FINER)) { logger.finer("Attempting DNS resolution of " + host); } - InternalResolutionResult result = null; + ResolutionResult result = null; try { EquivalentAddressGroup proxiedAddr = detectProxy(); - ResolutionResult.Builder resolutionResultBuilder = ResolutionResult.newBuilder(); if (proxiedAddr != null) { if (logger.isLoggable(Level.FINER)) { logger.finer("Using proxy address " + proxiedAddr); } - resolutionResultBuilder.setAddressesOrError( - StatusOr.fromValue(Collections.singletonList(proxiedAddr))); + result = ResolutionResult.newBuilder() + .setAddressesOrError(StatusOr.fromValue(Collections.singletonList(proxiedAddr))) + .build(); } else { - result = doResolve(false); - if (result.error != null) { - InternalResolutionResult finalResult = result; - syncContext.execute(() -> - savedListener.onResult2(ResolutionResult.newBuilder() - .setAddressesOrError(StatusOr.fromStatus(finalResult.error)) - .build())); - return; - } - if (result.addresses != null) { - resolutionResultBuilder.setAddressesOrError(StatusOr.fromValue(result.addresses)); - } - if (result.config != null) { - resolutionResultBuilder.setServiceConfig(result.config); - } - if (result.attributes != null) { - resolutionResultBuilder.setAttributes(result.attributes); - } + result = doResolve(); } + ResolutionResult savedResult = result; syncContext.execute(() -> { - savedListener.onResult2(resolutionResultBuilder.build()); + savedListener.onResult2(savedResult); }); } catch (IOException e) { syncContext.execute(() -> @@ -333,7 +313,7 @@ public void run() { Status.UNAVAILABLE.withDescription( "Unable to resolve host " + host).withCause(e))).build())); } finally { - final boolean succeed = result != null && result.error == null; + final boolean succeed = result != null && result.getAddressesOrError().hasValue(); syncContext.execute(new Runnable() { @Override public void run() { @@ -533,18 +513,6 @@ private static long getNetworkAddressCacheTtlNanos(boolean isAndroid) { return sc; } - /** - * Used as a DNS-based name resolver's internal representation of resolution result. - */ - protected static final class InternalResolutionResult { - private Status error; - private List addresses; - private ConfigOrError config; - public Attributes attributes; - - private InternalResolutionResult() {} - } - /** * Describes a parsed SRV record. */ diff --git a/core/src/test/java/io/grpc/internal/DnsNameResolverTest.java b/core/src/test/java/io/grpc/internal/DnsNameResolverTest.java index 0a46b7d8204..924e064bd94 100644 --- a/core/src/test/java/io/grpc/internal/DnsNameResolverTest.java +++ b/core/src/test/java/io/grpc/internal/DnsNameResolverTest.java @@ -695,7 +695,7 @@ public ConfigOrError parseServiceConfig(Map rawServiceConfig) { } @Test - public void resolve_addressFailure_neverLookUpServiceConfig() throws Exception { + public void resolve_addressFailure_stillLookUpServiceConfig() throws Exception { DnsNameResolver.enableTxt = true; AddressResolver mockAddressResolver = mock(AddressResolver.class); when(mockAddressResolver.resolveAddress(anyString())) @@ -714,7 +714,7 @@ public void resolve_addressFailure_neverLookUpServiceConfig() throws Exception { Status errorStatus = resultCaptor.getValue().getAddressesOrError().getStatus(); assertThat(errorStatus.getCode()).isEqualTo(Code.UNAVAILABLE); assertThat(errorStatus.getCause()).hasMessageThat().contains("no addr"); - verify(mockResourceResolver, never()).resolveTxt(anyString()); + verify(mockResourceResolver).resolveTxt("_grpc_config." + name); assertEquals(0, fakeClock.numPendingTasks()); // A retry should be scheduled diff --git a/grpclb/src/main/java/io/grpc/grpclb/GrpclbNameResolver.java b/grpclb/src/main/java/io/grpc/grpclb/GrpclbNameResolver.java index d17587fb14d..60d02220e64 100644 --- a/grpclb/src/main/java/io/grpc/grpclb/GrpclbNameResolver.java +++ b/grpclb/src/main/java/io/grpc/grpclb/GrpclbNameResolver.java @@ -21,6 +21,7 @@ import io.grpc.Attributes; import io.grpc.EquivalentAddressGroup; import io.grpc.NameResolver; +import io.grpc.StatusOr; import io.grpc.internal.DnsNameResolver; import io.grpc.internal.SharedResourceHolder.Resource; import java.net.InetAddress; @@ -58,14 +59,22 @@ final class GrpclbNameResolver extends DnsNameResolver { } @Override - protected InternalResolutionResult doResolve(boolean forceTxt) { + protected ResolutionResult doResolve() { + ResolutionResult result = super.doResolve(); List balancerAddrs = resolveBalancerAddresses(); - InternalResolutionResult result = super.doResolve(!balancerAddrs.isEmpty()); if (!balancerAddrs.isEmpty()) { - result.attributes = - Attributes.newBuilder() + ResolutionResult.Builder resultBuilder = result.toBuilder() + .setAttributes(result.getAttributes().toBuilder() .set(GrpclbConstants.ATTR_LB_ADDRS, balancerAddrs) - .build(); + .build()); + if (!result.getAddressesOrError().hasValue()) { + // While ResolutionResult is powerful enough to communicate attributes simultaneously with + // an address resolution failure, LoadBalancer.ResolvedAddresses isn't yet and so the + // attributes are lost if addresses fail. GrpclbLB will be able to handle the lack of + // addresses since there are LB addresses, so discard the failure for now. + resultBuilder.setAddressesOrError(StatusOr.fromValue(Collections.emptyList())); + } + result = resultBuilder.build(); } return result; } diff --git a/grpclb/src/test/java/io/grpc/grpclb/GrpclbNameResolverTest.java b/grpclb/src/test/java/io/grpc/grpclb/GrpclbNameResolverTest.java index 1aa11ecf9af..a90556a01b0 100644 --- a/grpclb/src/test/java/io/grpc/grpclb/GrpclbNameResolverTest.java +++ b/grpclb/src/test/java/io/grpc/grpclb/GrpclbNameResolverTest.java @@ -20,7 +20,6 @@ import static org.mockito.ArgumentMatchers.anyString; import static org.mockito.Mockito.lenient; import static org.mockito.Mockito.mock; -import static org.mockito.Mockito.never; import static org.mockito.Mockito.verify; import static org.mockito.Mockito.when; @@ -319,7 +318,7 @@ public void resolveAll_balancerLookupFails_stillLookUpServiceConfig() throws Exc } @Test - public void resolve_addressAndBalancersLookupFail_neverLookupServiceConfig() throws Exception { + public void resolve_addressAndBalancersLookupFail_stillLookupServiceConfig() throws Exception { AddressResolver mockAddressResolver = mock(AddressResolver.class); when(mockAddressResolver.resolveAddress(anyString())) .thenThrow(new UnknownHostException("I really tried")); @@ -338,7 +337,7 @@ public void resolve_addressAndBalancersLookupFail_neverLookupServiceConfig() thr Status errorStatus = resultCaptor.getValue().getAddressesOrError().getStatus(); assertThat(errorStatus.getCode()).isEqualTo(Code.UNAVAILABLE); verify(mockAddressResolver).resolveAddress(hostName); - verify(mockResourceResolver, never()).resolveTxt("_grpc_config." + hostName); + verify(mockResourceResolver).resolveTxt("_grpc_config." + hostName); verify(mockResourceResolver).resolveSrv("_grpclb._tcp." + hostName); } }