From 314c20758edfb053f35ef47a8cb048deb68e1a2b Mon Sep 17 00:00:00 2001 From: Eric Anderson Date: Mon, 9 Mar 2026 18:22:02 -0700 Subject: [PATCH] Remove InternalResolutionResult from DnsNameResolver The internal result was needed before 90d0fabb1 allowed addresses to fail yet still provide attributes and service config. Now the code can just use the regular API. This does cause a behavior change where TXT records are looked up even if address lookup failed, however that's actually what we wanted to allow in 90d0fabb1 by adding the new API. Also, the TXT code was added in 2017 and it's now 2026 yet it is still disabled, so it's unlikely to matter soon. --- .../io/grpc/internal/DnsNameResolver.java | 62 +++++-------------- .../io/grpc/internal/DnsNameResolverTest.java | 4 +- .../io/grpc/grpclb/GrpclbNameResolver.java | 19 ++++-- .../grpc/grpclb/GrpclbNameResolverTest.java | 5 +- 4 files changed, 33 insertions(+), 57 deletions(-) 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); } }