Remove InternalResolutionResult from DnsNameResolver

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<EquivalentAddressGroup> addresses;
-    private ConfigOrError config;
-    public Attributes attributes;
-
-    private InternalResolutionResult() {}
-  }
-
   /**
    * Describes a parsed SRV record.
    */

core/src/test/java/io/grpc/internal/DnsNameResolverTest.java

@@ -695,7 +695,7 @@ public ConfigOrError parseServiceConfig(Map<String, ?> 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

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<EquivalentAddressGroup> 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;
   }

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);
   }
 }