From 707e1da5efe648ec3b1aeb231b3c1d3971027506 Mon Sep 17 00:00:00 2001 From: John Cormie Date: Tue, 5 Nov 2024 11:04:11 -0800 Subject: [PATCH 01/23] Introduce NameResolver.Args extensions. --- .../io/grpc/ForwardingChannelBuilder2.java | 7 + .../java/io/grpc/ManagedChannelBuilder.java | 12 ++ api/src/main/java/io/grpc/NameResolver.java | 179 +++++++++++++++++- .../io/grpc/internal/ManagedChannelImpl.java | 1 + .../internal/ManagedChannelImplBuilder.java | 9 + .../grpc/internal/ManagedChannelImplTest.java | 9 +- 6 files changed, 214 insertions(+), 3 deletions(-) diff --git a/api/src/main/java/io/grpc/ForwardingChannelBuilder2.java b/api/src/main/java/io/grpc/ForwardingChannelBuilder2.java index 7f21a57ec80..395cfd27187 100644 --- a/api/src/main/java/io/grpc/ForwardingChannelBuilder2.java +++ b/api/src/main/java/io/grpc/ForwardingChannelBuilder2.java @@ -18,6 +18,7 @@ import com.google.common.base.MoreObjects; import com.google.errorprone.annotations.DoNotCall; +import io.grpc.NameResolver.Args; import java.util.List; import java.util.Map; import java.util.concurrent.Executor; @@ -263,6 +264,12 @@ protected T addMetricSink(MetricSink metricSink) { return thisT(); } + @Override + public T setNameResolverArg(Args.Key key, X value) { + delegate().setNameResolverArg(key, value); + return thisT(); + } + /** * Returns the {@link ManagedChannel} built by the delegate by default. Overriding method can * return different value. diff --git a/api/src/main/java/io/grpc/ManagedChannelBuilder.java b/api/src/main/java/io/grpc/ManagedChannelBuilder.java index 6e30d8eae04..1c4a29a73c0 100644 --- a/api/src/main/java/io/grpc/ManagedChannelBuilder.java +++ b/api/src/main/java/io/grpc/ManagedChannelBuilder.java @@ -17,6 +17,7 @@ package io.grpc; import com.google.common.base.Preconditions; +import io.grpc.NameResolver.Args; import java.util.List; import java.util.Map; import java.util.concurrent.Executor; @@ -633,6 +634,17 @@ protected T addMetricSink(MetricSink metricSink) { throw new UnsupportedOperationException(); } + /** + * Adds an "extra" externally-defined argument for the name resolver, if any. + * + * @param key identifies the argument in a type-safe manner + * @param value the extra argument + * @return this + */ + @ExperimentalApi("https://github.com/grpc/grpc-java/issues/00000") + public T setNameResolverArg(Args.Key key, X value) { + throw new UnsupportedOperationException(); + } /** * Builds a channel using the given parameters. diff --git a/api/src/main/java/io/grpc/NameResolver.java b/api/src/main/java/io/grpc/NameResolver.java index b9590ab5d5a..4c6e62621bb 100644 --- a/api/src/main/java/io/grpc/NameResolver.java +++ b/api/src/main/java/io/grpc/NameResolver.java @@ -28,11 +28,14 @@ import java.lang.annotation.RetentionPolicy; import java.net.URI; import java.util.Collections; +import java.util.IdentityHashMap; import java.util.List; import java.util.Map; +import java.util.Set; import java.util.concurrent.Executor; import java.util.concurrent.ScheduledExecutorService; import javax.annotation.Nullable; +import javax.annotation.concurrent.Immutable; import javax.annotation.concurrent.ThreadSafe; /** @@ -274,7 +277,20 @@ public Status onResult2(ResolutionResult resolutionResult) { /** * Information that a {@link Factory} uses to create a {@link NameResolver}. * - *

Note this class doesn't override neither {@code equals()} nor {@code hashCode()}. + *

Args applicable to all {@link NameResolver}s are defined here using ordinary setters and + * getters. This container can additionally hold externally-defined "extended" args that aren't + * universally useful. An extended arg may be appropriate when it's only meaningful for a certain + * URI scheme, a certain {@link java.net.SocketAddress}, a specific {@link NameResolver} subclass + * or in other cases where a direct dependency from here would be undesirable. Extended args are + * identified by {@link Args.Key} which should be defined as a static constant in a java package + * and class appropriate to it's scope. + * + *

The address to be resolved is provided to a {@link NameResolver} as a URI while {@link Args} + * are normally reserved for supporting dependencies. However, extended args are very useful for + * passing auxiliary addressing information that can't be encoded as a string or can't be part of the URI + * for other reasons. + * + *

Note this class overrides neither {@code equals()} nor {@code hashCode()}. * * @since 1.21.0 */ @@ -284,6 +300,7 @@ public static final class Args { private final ProxyDetector proxyDetector; private final SynchronizationContext syncContext; private final ServiceConfigParser serviceConfigParser; + private final Extensions extras; @Nullable private final ScheduledExecutorService scheduledExecutorService; @Nullable private final ChannelLogger channelLogger; @Nullable private final Executor executor; @@ -294,6 +311,7 @@ private Args( ProxyDetector proxyDetector, SynchronizationContext syncContext, ServiceConfigParser serviceConfigParser, + Extensions extras, @Nullable ScheduledExecutorService scheduledExecutorService, @Nullable ChannelLogger channelLogger, @Nullable Executor executor, @@ -302,6 +320,7 @@ private Args( this.proxyDetector = checkNotNull(proxyDetector, "proxyDetector not set"); this.syncContext = checkNotNull(syncContext, "syncContext not set"); this.serviceConfigParser = checkNotNull(serviceConfigParser, "serviceConfigParser not set"); + this.extras = checkNotNull(extras, "extras not set"); this.scheduledExecutorService = scheduledExecutorService; this.channelLogger = channelLogger; this.executor = executor; @@ -366,6 +385,24 @@ public ServiceConfigParser getServiceConfigParser() { return serviceConfigParser; } + /** + * Gets the value for an extra by key, or {@code null} if it's not present. + */ + @SuppressWarnings("unchecked") + @Nullable + public T getExtension(Key key) { + return extras.get(key); + } + + /** + * Returns the set of externally-defined "extra" args. + */ + @SuppressWarnings("unchecked") + @Internal + public Extensions getExtras() { + return extras; + } + /** * Returns the {@link ChannelLogger} for the Channel served by this NameResolver. * @@ -411,6 +448,7 @@ public String toString() { .add("proxyDetector", proxyDetector) .add("syncContext", syncContext) .add("serviceConfigParser", serviceConfigParser) + .add("extraArgs", extras) .add("scheduledExecutorService", scheduledExecutorService) .add("channelLogger", channelLogger) .add("executor", executor) @@ -429,6 +467,7 @@ public Builder toBuilder() { builder.setProxyDetector(proxyDetector); builder.setSynchronizationContext(syncContext); builder.setServiceConfigParser(serviceConfigParser); + builder.setExtensions(extras); builder.setScheduledExecutorService(scheduledExecutorService); builder.setChannelLogger(channelLogger); builder.setOffloadExecutor(executor); @@ -459,6 +498,7 @@ public static final class Builder { private ChannelLogger channelLogger; private Executor executor; private String overrideAuthority; + private Extensions extensions = Extensions.EMPTY; Builder() { } @@ -545,6 +585,14 @@ public Builder setOverrideAuthority(String authority) { return this; } + /** + * See {@link Args#getExtension(Key)}. + */ + public Builder setExtensions(Extensions extensions) { + this.extensions = extensions; + return this; + } + /** * Builds an {@link Args}. * @@ -553,10 +601,137 @@ public Builder setOverrideAuthority(String authority) { public Args build() { return new Args( - defaultPort, proxyDetector, syncContext, serviceConfigParser, + defaultPort, proxyDetector, syncContext, serviceConfigParser, extensions, scheduledExecutorService, channelLogger, executor, overrideAuthority); } } + + /** + * Identifies an externally-defined {@link Args} extension. + * + *

Uses reference equality. + * + * @param type of the value in the key-value pair + */ + @Immutable + @SuppressWarnings("UnusedTypeParameter") + public static final class Key { + private final String debugString; + + private Key(String debugString) { + this.debugString = debugString; + } + + @Override + public String toString() { + return debugString; + } + + /** + * Factory method for creating instances of {@link Key}. + * + * @param debugString a string used to describe the key, used for debugging. + * @param Key type + * @return Key object + */ + public static Key create(String debugString) { + return new Key<>(debugString); + } + } + + /** + * An immutable type-safe container of externally-defined {@link NameResolver} arguments. + * + *

NB: This class overrides neither {@code equals()} nor {@code hashCode()}. + */ + @ExperimentalApi("https://github.com/grpc/grpc-java/issues/0000") + @Immutable + public static final class Extensions { + private static final IdentityHashMap, Object> EMPTY_MAP = new IdentityHashMap<>(); + + public static final Extensions EMPTY = new Extensions(EMPTY_MAP); + + private final IdentityHashMap, Object> data; + private Extensions(IdentityHashMap, Object> data) { + assert data != null; + this.data = data; + } + + /** + * Gets the value for the key, or {@code null} if it's not present. + */ + @SuppressWarnings("unchecked") + @Nullable + public T get(Key key) { + return (T) data.get(key); + } + + /** + * Creates a new builder. + */ + public static Builder newBuilder() { + return new Builder(EMPTY); + } + + /** + * Creates a new builder that is pre-populated with the content of this container. + * @return a new builder. + */ + public Builder toBuilder() { + return new Builder(this); + } + + @Override + public String toString() { + return data.toString(); + } + + /** + * Fluently builds an instance of {@link Extensions}. + */ + public static final class Builder { + private Extensions base; + private IdentityHashMap, Object> newdata; + + private Builder(Extensions base) { + assert base != null; + this.base = base; + } + + private IdentityHashMap, Object> data(int size) { + if (newdata == null) { + newdata = new IdentityHashMap<>(size); + } + return newdata; + } + + public Builder set(Key key, T value) { + data(1).put(key, value); + return this; + } + + public Builder setAll(Extensions other) { + data(other.data.size()).putAll(other.data); + return this; + } + + /** + * Build the extensions. + */ + public Extensions build() { + if (newdata != null) { + for (Map.Entry, Object> entry : base.data.entrySet()) { + if (!newdata.containsKey(entry.getKey())) { + newdata.put(entry.getKey(), entry.getValue()); + } + } + base = new Extensions(newdata); + newdata = null; + } + return base; + } + } + } } /** diff --git a/core/src/main/java/io/grpc/internal/ManagedChannelImpl.java b/core/src/main/java/io/grpc/internal/ManagedChannelImpl.java index b89a126517f..60fba3f1383 100644 --- a/core/src/main/java/io/grpc/internal/ManagedChannelImpl.java +++ b/core/src/main/java/io/grpc/internal/ManagedChannelImpl.java @@ -599,6 +599,7 @@ ClientStream newSubstream( .setChannelLogger(channelLogger) .setOffloadExecutor(this.offloadExecutorHolder) .setOverrideAuthority(this.authorityOverride) + .setExtensions(builder.resolverExArgsBuilder.build()) .build(); this.nameResolver = getNameResolver( targetUri, authorityOverride, nameResolverProvider, nameResolverArgs); diff --git a/core/src/main/java/io/grpc/internal/ManagedChannelImplBuilder.java b/core/src/main/java/io/grpc/internal/ManagedChannelImplBuilder.java index 48a255472e1..350b87f2599 100644 --- a/core/src/main/java/io/grpc/internal/ManagedChannelImplBuilder.java +++ b/core/src/main/java/io/grpc/internal/ManagedChannelImplBuilder.java @@ -198,6 +198,9 @@ public static ManagedChannelBuilder forTarget(String target) { @Nullable ProxyDetector proxyDetector; + NameResolver.Args.Extensions.Builder resolverExArgsBuilder = + NameResolver.Args.Extensions.newBuilder(); + private boolean authorityCheckerDisabled; private boolean statsEnabled = true; private boolean recordStartedRpcs = true; @@ -554,6 +557,12 @@ public ManagedChannelImplBuilder defaultServiceConfig(@Nullable Map s return this; } + @Override + public ManagedChannelImplBuilder setNameResolverArg(NameResolver.Args.Key key, X value) { + resolverExArgsBuilder.set(key, value); + return this; + } + @Nullable private static Map checkMapEntryTypes(@Nullable Map map) { if (map == null) { diff --git a/core/src/test/java/io/grpc/internal/ManagedChannelImplTest.java b/core/src/test/java/io/grpc/internal/ManagedChannelImplTest.java index 16700096827..9976b69dce8 100644 --- a/core/src/test/java/io/grpc/internal/ManagedChannelImplTest.java +++ b/core/src/test/java/io/grpc/internal/ManagedChannelImplTest.java @@ -204,6 +204,8 @@ public class ManagedChannelImplTest { .setServiceConfigParser(mock(NameResolver.ServiceConfigParser.class)) .setScheduledExecutorService(new FakeClock().getScheduledExecutorService()) .build(); + private static final NameResolver.Args.Key TEST_RESOLVER_ARG_EXT_KEY = + NameResolver.Args.Key.create("test-key"); private URI expectedUri; private final SocketAddress socketAddress = @@ -4243,13 +4245,18 @@ public String getDefaultScheme() { return "fake"; } }; - channelBuilder.nameResolverFactory(factory).proxyDetector(neverProxy); + channelBuilder + .nameResolverFactory(factory) + .proxyDetector(neverProxy) + .setNameResolverArg(TEST_RESOLVER_ARG_EXT_KEY, "test-value"); + createChannel(); NameResolver.Args args = capturedArgs.get(); assertThat(args).isNotNull(); assertThat(args.getDefaultPort()).isEqualTo(DEFAULT_PORT); assertThat(args.getProxyDetector()).isSameInstanceAs(neverProxy); + assertThat(args.getExtension(TEST_RESOLVER_ARG_EXT_KEY)).isEqualTo("test-value"); verify(offloadExecutor, never()).execute(any(Runnable.class)); args.getOffloadExecutor() From 54e089b69c917e553ced380664bdf5dec9094b2c Mon Sep 17 00:00:00 2001 From: John Cormie Date: Tue, 5 Nov 2024 11:04:11 -0800 Subject: [PATCH 02/23] Introduce NameResolver.Args extensions. --- .../io/grpc/ForwardingChannelBuilder2.java | 7 + .../java/io/grpc/ManagedChannelBuilder.java | 12 ++ api/src/main/java/io/grpc/NameResolver.java | 179 +++++++++++++++++- .../io/grpc/internal/ManagedChannelImpl.java | 1 + .../internal/ManagedChannelImplBuilder.java | 9 + .../grpc/internal/ManagedChannelImplTest.java | 9 +- 6 files changed, 214 insertions(+), 3 deletions(-) diff --git a/api/src/main/java/io/grpc/ForwardingChannelBuilder2.java b/api/src/main/java/io/grpc/ForwardingChannelBuilder2.java index 7f21a57ec80..395cfd27187 100644 --- a/api/src/main/java/io/grpc/ForwardingChannelBuilder2.java +++ b/api/src/main/java/io/grpc/ForwardingChannelBuilder2.java @@ -18,6 +18,7 @@ import com.google.common.base.MoreObjects; import com.google.errorprone.annotations.DoNotCall; +import io.grpc.NameResolver.Args; import java.util.List; import java.util.Map; import java.util.concurrent.Executor; @@ -263,6 +264,12 @@ protected T addMetricSink(MetricSink metricSink) { return thisT(); } + @Override + public T setNameResolverArg(Args.Key key, X value) { + delegate().setNameResolverArg(key, value); + return thisT(); + } + /** * Returns the {@link ManagedChannel} built by the delegate by default. Overriding method can * return different value. diff --git a/api/src/main/java/io/grpc/ManagedChannelBuilder.java b/api/src/main/java/io/grpc/ManagedChannelBuilder.java index 6e30d8eae04..1c4a29a73c0 100644 --- a/api/src/main/java/io/grpc/ManagedChannelBuilder.java +++ b/api/src/main/java/io/grpc/ManagedChannelBuilder.java @@ -17,6 +17,7 @@ package io.grpc; import com.google.common.base.Preconditions; +import io.grpc.NameResolver.Args; import java.util.List; import java.util.Map; import java.util.concurrent.Executor; @@ -633,6 +634,17 @@ protected T addMetricSink(MetricSink metricSink) { throw new UnsupportedOperationException(); } + /** + * Adds an "extra" externally-defined argument for the name resolver, if any. + * + * @param key identifies the argument in a type-safe manner + * @param value the extra argument + * @return this + */ + @ExperimentalApi("https://github.com/grpc/grpc-java/issues/00000") + public T setNameResolverArg(Args.Key key, X value) { + throw new UnsupportedOperationException(); + } /** * Builds a channel using the given parameters. diff --git a/api/src/main/java/io/grpc/NameResolver.java b/api/src/main/java/io/grpc/NameResolver.java index b9590ab5d5a..4c6e62621bb 100644 --- a/api/src/main/java/io/grpc/NameResolver.java +++ b/api/src/main/java/io/grpc/NameResolver.java @@ -28,11 +28,14 @@ import java.lang.annotation.RetentionPolicy; import java.net.URI; import java.util.Collections; +import java.util.IdentityHashMap; import java.util.List; import java.util.Map; +import java.util.Set; import java.util.concurrent.Executor; import java.util.concurrent.ScheduledExecutorService; import javax.annotation.Nullable; +import javax.annotation.concurrent.Immutable; import javax.annotation.concurrent.ThreadSafe; /** @@ -274,7 +277,20 @@ public Status onResult2(ResolutionResult resolutionResult) { /** * Information that a {@link Factory} uses to create a {@link NameResolver}. * - *

Note this class doesn't override neither {@code equals()} nor {@code hashCode()}. + *

Args applicable to all {@link NameResolver}s are defined here using ordinary setters and + * getters. This container can additionally hold externally-defined "extended" args that aren't + * universally useful. An extended arg may be appropriate when it's only meaningful for a certain + * URI scheme, a certain {@link java.net.SocketAddress}, a specific {@link NameResolver} subclass + * or in other cases where a direct dependency from here would be undesirable. Extended args are + * identified by {@link Args.Key} which should be defined as a static constant in a java package + * and class appropriate to it's scope. + * + *

The address to be resolved is provided to a {@link NameResolver} as a URI while {@link Args} + * are normally reserved for supporting dependencies. However, extended args are very useful for + * passing auxiliary addressing information that can't be encoded as a string or can't be part of the URI + * for other reasons. + * + *

Note this class overrides neither {@code equals()} nor {@code hashCode()}. * * @since 1.21.0 */ @@ -284,6 +300,7 @@ public static final class Args { private final ProxyDetector proxyDetector; private final SynchronizationContext syncContext; private final ServiceConfigParser serviceConfigParser; + private final Extensions extras; @Nullable private final ScheduledExecutorService scheduledExecutorService; @Nullable private final ChannelLogger channelLogger; @Nullable private final Executor executor; @@ -294,6 +311,7 @@ private Args( ProxyDetector proxyDetector, SynchronizationContext syncContext, ServiceConfigParser serviceConfigParser, + Extensions extras, @Nullable ScheduledExecutorService scheduledExecutorService, @Nullable ChannelLogger channelLogger, @Nullable Executor executor, @@ -302,6 +320,7 @@ private Args( this.proxyDetector = checkNotNull(proxyDetector, "proxyDetector not set"); this.syncContext = checkNotNull(syncContext, "syncContext not set"); this.serviceConfigParser = checkNotNull(serviceConfigParser, "serviceConfigParser not set"); + this.extras = checkNotNull(extras, "extras not set"); this.scheduledExecutorService = scheduledExecutorService; this.channelLogger = channelLogger; this.executor = executor; @@ -366,6 +385,24 @@ public ServiceConfigParser getServiceConfigParser() { return serviceConfigParser; } + /** + * Gets the value for an extra by key, or {@code null} if it's not present. + */ + @SuppressWarnings("unchecked") + @Nullable + public T getExtension(Key key) { + return extras.get(key); + } + + /** + * Returns the set of externally-defined "extra" args. + */ + @SuppressWarnings("unchecked") + @Internal + public Extensions getExtras() { + return extras; + } + /** * Returns the {@link ChannelLogger} for the Channel served by this NameResolver. * @@ -411,6 +448,7 @@ public String toString() { .add("proxyDetector", proxyDetector) .add("syncContext", syncContext) .add("serviceConfigParser", serviceConfigParser) + .add("extraArgs", extras) .add("scheduledExecutorService", scheduledExecutorService) .add("channelLogger", channelLogger) .add("executor", executor) @@ -429,6 +467,7 @@ public Builder toBuilder() { builder.setProxyDetector(proxyDetector); builder.setSynchronizationContext(syncContext); builder.setServiceConfigParser(serviceConfigParser); + builder.setExtensions(extras); builder.setScheduledExecutorService(scheduledExecutorService); builder.setChannelLogger(channelLogger); builder.setOffloadExecutor(executor); @@ -459,6 +498,7 @@ public static final class Builder { private ChannelLogger channelLogger; private Executor executor; private String overrideAuthority; + private Extensions extensions = Extensions.EMPTY; Builder() { } @@ -545,6 +585,14 @@ public Builder setOverrideAuthority(String authority) { return this; } + /** + * See {@link Args#getExtension(Key)}. + */ + public Builder setExtensions(Extensions extensions) { + this.extensions = extensions; + return this; + } + /** * Builds an {@link Args}. * @@ -553,10 +601,137 @@ public Builder setOverrideAuthority(String authority) { public Args build() { return new Args( - defaultPort, proxyDetector, syncContext, serviceConfigParser, + defaultPort, proxyDetector, syncContext, serviceConfigParser, extensions, scheduledExecutorService, channelLogger, executor, overrideAuthority); } } + + /** + * Identifies an externally-defined {@link Args} extension. + * + *

Uses reference equality. + * + * @param type of the value in the key-value pair + */ + @Immutable + @SuppressWarnings("UnusedTypeParameter") + public static final class Key { + private final String debugString; + + private Key(String debugString) { + this.debugString = debugString; + } + + @Override + public String toString() { + return debugString; + } + + /** + * Factory method for creating instances of {@link Key}. + * + * @param debugString a string used to describe the key, used for debugging. + * @param Key type + * @return Key object + */ + public static Key create(String debugString) { + return new Key<>(debugString); + } + } + + /** + * An immutable type-safe container of externally-defined {@link NameResolver} arguments. + * + *

NB: This class overrides neither {@code equals()} nor {@code hashCode()}. + */ + @ExperimentalApi("https://github.com/grpc/grpc-java/issues/0000") + @Immutable + public static final class Extensions { + private static final IdentityHashMap, Object> EMPTY_MAP = new IdentityHashMap<>(); + + public static final Extensions EMPTY = new Extensions(EMPTY_MAP); + + private final IdentityHashMap, Object> data; + private Extensions(IdentityHashMap, Object> data) { + assert data != null; + this.data = data; + } + + /** + * Gets the value for the key, or {@code null} if it's not present. + */ + @SuppressWarnings("unchecked") + @Nullable + public T get(Key key) { + return (T) data.get(key); + } + + /** + * Creates a new builder. + */ + public static Builder newBuilder() { + return new Builder(EMPTY); + } + + /** + * Creates a new builder that is pre-populated with the content of this container. + * @return a new builder. + */ + public Builder toBuilder() { + return new Builder(this); + } + + @Override + public String toString() { + return data.toString(); + } + + /** + * Fluently builds an instance of {@link Extensions}. + */ + public static final class Builder { + private Extensions base; + private IdentityHashMap, Object> newdata; + + private Builder(Extensions base) { + assert base != null; + this.base = base; + } + + private IdentityHashMap, Object> data(int size) { + if (newdata == null) { + newdata = new IdentityHashMap<>(size); + } + return newdata; + } + + public Builder set(Key key, T value) { + data(1).put(key, value); + return this; + } + + public Builder setAll(Extensions other) { + data(other.data.size()).putAll(other.data); + return this; + } + + /** + * Build the extensions. + */ + public Extensions build() { + if (newdata != null) { + for (Map.Entry, Object> entry : base.data.entrySet()) { + if (!newdata.containsKey(entry.getKey())) { + newdata.put(entry.getKey(), entry.getValue()); + } + } + base = new Extensions(newdata); + newdata = null; + } + return base; + } + } + } } /** diff --git a/core/src/main/java/io/grpc/internal/ManagedChannelImpl.java b/core/src/main/java/io/grpc/internal/ManagedChannelImpl.java index b89a126517f..60fba3f1383 100644 --- a/core/src/main/java/io/grpc/internal/ManagedChannelImpl.java +++ b/core/src/main/java/io/grpc/internal/ManagedChannelImpl.java @@ -599,6 +599,7 @@ ClientStream newSubstream( .setChannelLogger(channelLogger) .setOffloadExecutor(this.offloadExecutorHolder) .setOverrideAuthority(this.authorityOverride) + .setExtensions(builder.resolverExArgsBuilder.build()) .build(); this.nameResolver = getNameResolver( targetUri, authorityOverride, nameResolverProvider, nameResolverArgs); diff --git a/core/src/main/java/io/grpc/internal/ManagedChannelImplBuilder.java b/core/src/main/java/io/grpc/internal/ManagedChannelImplBuilder.java index 48a255472e1..350b87f2599 100644 --- a/core/src/main/java/io/grpc/internal/ManagedChannelImplBuilder.java +++ b/core/src/main/java/io/grpc/internal/ManagedChannelImplBuilder.java @@ -198,6 +198,9 @@ public static ManagedChannelBuilder forTarget(String target) { @Nullable ProxyDetector proxyDetector; + NameResolver.Args.Extensions.Builder resolverExArgsBuilder = + NameResolver.Args.Extensions.newBuilder(); + private boolean authorityCheckerDisabled; private boolean statsEnabled = true; private boolean recordStartedRpcs = true; @@ -554,6 +557,12 @@ public ManagedChannelImplBuilder defaultServiceConfig(@Nullable Map s return this; } + @Override + public ManagedChannelImplBuilder setNameResolverArg(NameResolver.Args.Key key, X value) { + resolverExArgsBuilder.set(key, value); + return this; + } + @Nullable private static Map checkMapEntryTypes(@Nullable Map map) { if (map == null) { diff --git a/core/src/test/java/io/grpc/internal/ManagedChannelImplTest.java b/core/src/test/java/io/grpc/internal/ManagedChannelImplTest.java index 16700096827..9976b69dce8 100644 --- a/core/src/test/java/io/grpc/internal/ManagedChannelImplTest.java +++ b/core/src/test/java/io/grpc/internal/ManagedChannelImplTest.java @@ -204,6 +204,8 @@ public class ManagedChannelImplTest { .setServiceConfigParser(mock(NameResolver.ServiceConfigParser.class)) .setScheduledExecutorService(new FakeClock().getScheduledExecutorService()) .build(); + private static final NameResolver.Args.Key TEST_RESOLVER_ARG_EXT_KEY = + NameResolver.Args.Key.create("test-key"); private URI expectedUri; private final SocketAddress socketAddress = @@ -4243,13 +4245,18 @@ public String getDefaultScheme() { return "fake"; } }; - channelBuilder.nameResolverFactory(factory).proxyDetector(neverProxy); + channelBuilder + .nameResolverFactory(factory) + .proxyDetector(neverProxy) + .setNameResolverArg(TEST_RESOLVER_ARG_EXT_KEY, "test-value"); + createChannel(); NameResolver.Args args = capturedArgs.get(); assertThat(args).isNotNull(); assertThat(args.getDefaultPort()).isEqualTo(DEFAULT_PORT); assertThat(args.getProxyDetector()).isSameInstanceAs(neverProxy); + assertThat(args.getExtension(TEST_RESOLVER_ARG_EXT_KEY)).isEqualTo("test-value"); verify(offloadExecutor, never()).execute(any(Runnable.class)); args.getOffloadExecutor() From ce2930f1dd6cab24dbfcee94c0d0a2104b811853 Mon Sep 17 00:00:00 2001 From: John Cormie Date: Tue, 5 Nov 2024 11:10:59 -0800 Subject: [PATCH 03/23] Introduce NameResolver.Args extensions. --- api/src/main/java/io/grpc/ForwardingChannelBuilder2.java | 4 ++-- api/src/main/java/io/grpc/ManagedChannelBuilder.java | 4 ++-- api/src/main/java/io/grpc/NameResolver.java | 9 ++++++--- 3 files changed, 10 insertions(+), 7 deletions(-) diff --git a/api/src/main/java/io/grpc/ForwardingChannelBuilder2.java b/api/src/main/java/io/grpc/ForwardingChannelBuilder2.java index 395cfd27187..fca87e54090 100644 --- a/api/src/main/java/io/grpc/ForwardingChannelBuilder2.java +++ b/api/src/main/java/io/grpc/ForwardingChannelBuilder2.java @@ -18,7 +18,7 @@ import com.google.common.base.MoreObjects; import com.google.errorprone.annotations.DoNotCall; -import io.grpc.NameResolver.Args; +import io.grpc.NameResolver; import java.util.List; import java.util.Map; import java.util.concurrent.Executor; @@ -265,7 +265,7 @@ protected T addMetricSink(MetricSink metricSink) { } @Override - public T setNameResolverArg(Args.Key key, X value) { + public T setNameResolverArg(NameResolver.Args.Key key, X value) { delegate().setNameResolverArg(key, value); return thisT(); } diff --git a/api/src/main/java/io/grpc/ManagedChannelBuilder.java b/api/src/main/java/io/grpc/ManagedChannelBuilder.java index 1c4a29a73c0..969c048051f 100644 --- a/api/src/main/java/io/grpc/ManagedChannelBuilder.java +++ b/api/src/main/java/io/grpc/ManagedChannelBuilder.java @@ -17,7 +17,7 @@ package io.grpc; import com.google.common.base.Preconditions; -import io.grpc.NameResolver.Args; +import io.grpc.NameResolver; import java.util.List; import java.util.Map; import java.util.concurrent.Executor; @@ -642,7 +642,7 @@ protected T addMetricSink(MetricSink metricSink) { * @return this */ @ExperimentalApi("https://github.com/grpc/grpc-java/issues/00000") - public T setNameResolverArg(Args.Key key, X value) { + public T setNameResolverArg(NameResolver.Args.Key key, X value) { throw new UnsupportedOperationException(); } diff --git a/api/src/main/java/io/grpc/NameResolver.java b/api/src/main/java/io/grpc/NameResolver.java index 4c6e62621bb..c37e3670bd5 100644 --- a/api/src/main/java/io/grpc/NameResolver.java +++ b/api/src/main/java/io/grpc/NameResolver.java @@ -31,7 +31,6 @@ import java.util.IdentityHashMap; import java.util.List; import java.util.Map; -import java.util.Set; import java.util.concurrent.Executor; import java.util.concurrent.ScheduledExecutorService; import javax.annotation.Nullable; @@ -287,8 +286,8 @@ public Status onResult2(ResolutionResult resolutionResult) { * *

The address to be resolved is provided to a {@link NameResolver} as a URI while {@link Args} * are normally reserved for supporting dependencies. However, extended args are very useful for - * passing auxiliary addressing information that can't be encoded as a string or can't be part of the URI - * for other reasons. + * passing auxiliary addressing information that can't be encoded as a string or can't be part of + * the URI for other reasons. * *

Note this class overrides neither {@code equals()} nor {@code hashCode()}. * @@ -649,9 +648,13 @@ public static Key create(String debugString) { public static final class Extensions { private static final IdentityHashMap, Object> EMPTY_MAP = new IdentityHashMap<>(); + /** + * The canonical empty instance. + */ public static final Extensions EMPTY = new Extensions(EMPTY_MAP); private final IdentityHashMap, Object> data; + private Extensions(IdentityHashMap, Object> data) { assert data != null; this.data = data; From 2f3077179e6a538857ef9f718c86119ba1e71632 Mon Sep 17 00:00:00 2001 From: John Cormie Date: Tue, 5 Nov 2024 12:33:09 -0800 Subject: [PATCH 04/23] fixes --- .../java/io/grpc/ManagedChannelBuilder.java | 4 +- api/src/main/java/io/grpc/NameResolver.java | 52 +++++++++---------- .../ManagedChannelImplBuilderTest.java | 9 ++++ 3 files changed, 36 insertions(+), 29 deletions(-) diff --git a/api/src/main/java/io/grpc/ManagedChannelBuilder.java b/api/src/main/java/io/grpc/ManagedChannelBuilder.java index 969c048051f..c0dde14c3ac 100644 --- a/api/src/main/java/io/grpc/ManagedChannelBuilder.java +++ b/api/src/main/java/io/grpc/ManagedChannelBuilder.java @@ -635,10 +635,10 @@ protected T addMetricSink(MetricSink metricSink) { } /** - * Adds an "extra" externally-defined argument for the name resolver, if any. + * Provides an extended argument for the name resolver, if any. * * @param key identifies the argument in a type-safe manner - * @param value the extra argument + * @param value the argument itself * @return this */ @ExperimentalApi("https://github.com/grpc/grpc-java/issues/00000") diff --git a/api/src/main/java/io/grpc/NameResolver.java b/api/src/main/java/io/grpc/NameResolver.java index c37e3670bd5..317351bf205 100644 --- a/api/src/main/java/io/grpc/NameResolver.java +++ b/api/src/main/java/io/grpc/NameResolver.java @@ -277,17 +277,9 @@ public Status onResult2(ResolutionResult resolutionResult) { * Information that a {@link Factory} uses to create a {@link NameResolver}. * *

Args applicable to all {@link NameResolver}s are defined here using ordinary setters and - * getters. This container can additionally hold externally-defined "extended" args that aren't - * universally useful. An extended arg may be appropriate when it's only meaningful for a certain - * URI scheme, a certain {@link java.net.SocketAddress}, a specific {@link NameResolver} subclass - * or in other cases where a direct dependency from here would be undesirable. Extended args are - * identified by {@link Args.Key} which should be defined as a static constant in a java package - * and class appropriate to it's scope. - * - *

The address to be resolved is provided to a {@link NameResolver} as a URI while {@link Args} - * are normally reserved for supporting dependencies. However, extended args are very useful for - * passing auxiliary addressing information that can't be encoded as a string or can't be part of - * the URI for other reasons. + * getters. This container can also hold externally-defined "extended" args that aren't so widely + * useful or to avoid adding dependencies to this low level class. See {@link Args.Extensions} for + * more. * *

Note this class overrides neither {@code equals()} nor {@code hashCode()}. * @@ -299,7 +291,7 @@ public static final class Args { private final ProxyDetector proxyDetector; private final SynchronizationContext syncContext; private final ServiceConfigParser serviceConfigParser; - private final Extensions extras; + private final Extensions extensions; @Nullable private final ScheduledExecutorService scheduledExecutorService; @Nullable private final ChannelLogger channelLogger; @Nullable private final Executor executor; @@ -310,7 +302,7 @@ private Args( ProxyDetector proxyDetector, SynchronizationContext syncContext, ServiceConfigParser serviceConfigParser, - Extensions extras, + Extensions extensions, @Nullable ScheduledExecutorService scheduledExecutorService, @Nullable ChannelLogger channelLogger, @Nullable Executor executor, @@ -319,7 +311,7 @@ private Args( this.proxyDetector = checkNotNull(proxyDetector, "proxyDetector not set"); this.syncContext = checkNotNull(syncContext, "syncContext not set"); this.serviceConfigParser = checkNotNull(serviceConfigParser, "serviceConfigParser not set"); - this.extras = checkNotNull(extras, "extras not set"); + this.extensions = checkNotNull(extensions, "extras not set"); this.scheduledExecutorService = scheduledExecutorService; this.channelLogger = channelLogger; this.executor = executor; @@ -385,21 +377,12 @@ public ServiceConfigParser getServiceConfigParser() { } /** - * Gets the value for an extra by key, or {@code null} if it's not present. + * Gets the value of an extended arg by key, or {@code null} if it's not set. */ @SuppressWarnings("unchecked") @Nullable public T getExtension(Key key) { - return extras.get(key); - } - - /** - * Returns the set of externally-defined "extra" args. - */ - @SuppressWarnings("unchecked") - @Internal - public Extensions getExtras() { - return extras; + return extensions.get(key); } /** @@ -447,7 +430,7 @@ public String toString() { .add("proxyDetector", proxyDetector) .add("syncContext", syncContext) .add("serviceConfigParser", serviceConfigParser) - .add("extraArgs", extras) + .add("extensions", extensions) .add("scheduledExecutorService", scheduledExecutorService) .add("channelLogger", channelLogger) .add("executor", executor) @@ -466,7 +449,7 @@ public Builder toBuilder() { builder.setProxyDetector(proxyDetector); builder.setSynchronizationContext(syncContext); builder.setServiceConfigParser(serviceConfigParser); - builder.setExtensions(extras); + builder.setExtensions(extensions); builder.setScheduledExecutorService(scheduledExecutorService); builder.setChannelLogger(channelLogger); builder.setOffloadExecutor(executor); @@ -641,6 +624,21 @@ public static Key create(String debugString) { /** * An immutable type-safe container of externally-defined {@link NameResolver} arguments. * + *

While ordinary {@link Args} should be universally useful and meaningful, extended args can + * apply just to resolvers of a certain URI scheme, just to resolvers producing a particular + * type of {@link java.net.SocketAddress}, or even a individual {@link NameResolver} subclass. + * Extended args are identified by {@link Args.Key} which should be defined as a static constant + * in a java package and class appropriate to the argument's scope. + * + *

{@link Args} are normally reserved for information in support of name resolution, not the + * actual address to be resolved. However, there are rare cases where some or all of the input + * address can't be represented by any standard URI scheme or encoded as a String at all. + * Extension args can be a useful work around in these cases because they can hold an arbitrary + * Java type. + * + *

Extensions can also simply be used to avoid adding inappropriate deps to the low level + * io.grpc package. + * *

NB: This class overrides neither {@code equals()} nor {@code hashCode()}. */ @ExperimentalApi("https://github.com/grpc/grpc-java/issues/0000") diff --git a/core/src/test/java/io/grpc/internal/ManagedChannelImplBuilderTest.java b/core/src/test/java/io/grpc/internal/ManagedChannelImplBuilderTest.java index ddef7ef546f..72eb57cc142 100644 --- a/core/src/test/java/io/grpc/internal/ManagedChannelImplBuilderTest.java +++ b/core/src/test/java/io/grpc/internal/ManagedChannelImplBuilderTest.java @@ -764,6 +764,15 @@ public void disableNameResolverServiceConfig() { assertThat(builder.lookUpServiceConfig).isFalse(); } + @Test + public void setNameResolverExtArgs() { + assertThat(builder.resolverExArgsBuilder.build()) + .isSameInstanceAs(NameResolver.Args.Extensions.EMPTY); + NameResolver.Args.Key testKey = NameResolver.Args.Key.create("test-key"); + builder.setNameResolverArg(testKey, 42); + assertThat(builder.resolverExArgsBuilder.build().get(testKey)).isEqualTo(42); + } + @Test public void metricSinks() { MetricSink mocksink = mock(MetricSink.class); From 1a21575f7db59ca0a0d594eab0a613d997111df9 Mon Sep 17 00:00:00 2001 From: John Cormie Date: Tue, 5 Nov 2024 12:38:23 -0800 Subject: [PATCH 05/23] understand --- api/src/main/java/io/grpc/ManagedChannelBuilder.java | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/api/src/main/java/io/grpc/ManagedChannelBuilder.java b/api/src/main/java/io/grpc/ManagedChannelBuilder.java index c0dde14c3ac..e932e0f6daf 100644 --- a/api/src/main/java/io/grpc/ManagedChannelBuilder.java +++ b/api/src/main/java/io/grpc/ManagedChannelBuilder.java @@ -635,7 +635,10 @@ protected T addMetricSink(MetricSink metricSink) { } /** - * Provides an extended argument for the name resolver, if any. + * Provides an extended argument for the {@link NameResolver}, if applicable. + * + *

NB: If the selected {@link NameResolver} does not understand your extension, it will be + * silently ignored. * * @param key identifies the argument in a type-safe manner * @param value the argument itself From 07cb7c2b2112fccbb2c7daa67ece4e664179e736 Mon Sep 17 00:00:00 2001 From: John Cormie Date: Tue, 5 Nov 2024 12:41:04 -0800 Subject: [PATCH 06/23] appropriate --- api/src/main/java/io/grpc/NameResolver.java | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/api/src/main/java/io/grpc/NameResolver.java b/api/src/main/java/io/grpc/NameResolver.java index 317351bf205..868baef096b 100644 --- a/api/src/main/java/io/grpc/NameResolver.java +++ b/api/src/main/java/io/grpc/NameResolver.java @@ -278,8 +278,8 @@ public Status onResult2(ResolutionResult resolutionResult) { * *

Args applicable to all {@link NameResolver}s are defined here using ordinary setters and * getters. This container can also hold externally-defined "extended" args that aren't so widely - * useful or to avoid adding dependencies to this low level class. See {@link Args.Extensions} for - * more. + * useful or that would be inappropriate dependencies for this low level API. See {@link + * Args.Extensions} for more. * *

Note this class overrides neither {@code equals()} nor {@code hashCode()}. * From 169e9939f27796c7297ae0b10febef4fd453ed84 Mon Sep 17 00:00:00 2001 From: John Cormie Date: Tue, 5 Nov 2024 13:02:37 -0800 Subject: [PATCH 07/23] meaning --- api/src/main/java/io/grpc/NameResolver.java | 57 +++++++++------------ 1 file changed, 25 insertions(+), 32 deletions(-) diff --git a/api/src/main/java/io/grpc/NameResolver.java b/api/src/main/java/io/grpc/NameResolver.java index 868baef096b..64610968352 100644 --- a/api/src/main/java/io/grpc/NameResolver.java +++ b/api/src/main/java/io/grpc/NameResolver.java @@ -297,31 +297,25 @@ public static final class Args { @Nullable private final Executor executor; @Nullable private final String overrideAuthority; - private Args( - Integer defaultPort, - ProxyDetector proxyDetector, - SynchronizationContext syncContext, - ServiceConfigParser serviceConfigParser, - Extensions extensions, - @Nullable ScheduledExecutorService scheduledExecutorService, - @Nullable ChannelLogger channelLogger, - @Nullable Executor executor, - @Nullable String overrideAuthority) { - this.defaultPort = checkNotNull(defaultPort, "defaultPort not set"); - this.proxyDetector = checkNotNull(proxyDetector, "proxyDetector not set"); - this.syncContext = checkNotNull(syncContext, "syncContext not set"); - this.serviceConfigParser = checkNotNull(serviceConfigParser, "serviceConfigParser not set"); - this.extensions = checkNotNull(extensions, "extras not set"); - this.scheduledExecutorService = scheduledExecutorService; - this.channelLogger = channelLogger; - this.executor = executor; - this.overrideAuthority = overrideAuthority; + private Args(Builder builder) { + this.defaultPort = checkNotNull(builder.defaultPort, "defaultPort not set"); + this.proxyDetector = checkNotNull(builder.proxyDetector, "proxyDetector not set"); + this.syncContext = checkNotNull(builder.syncContext, "syncContext not set"); + this.serviceConfigParser = + checkNotNull(builder.serviceConfigParser, "serviceConfigParser not set"); + this.extensions = checkNotNull(builder.extensions, "extensions not set"); + this.scheduledExecutorService = builder.scheduledExecutorService; + this.channelLogger = builder.channelLogger; + this.executor = builder.executor; + this.overrideAuthority = builder.overrideAuthority; } /** * The port number used in case the target or the underlying naming system doesn't provide a * port number. * + *

TODO: Only meaningful for InetSocketAddress producers. Move to {@link Extensions}? + * * @since 1.21.0 */ public int getDefaultPort() { @@ -569,6 +563,8 @@ public Builder setOverrideAuthority(String authority) { /** * See {@link Args#getExtension(Key)}. + * + *

Optional, {@link Extensions#EMPTY} by default. */ public Builder setExtensions(Extensions extensions) { this.extensions = extensions; @@ -581,17 +577,14 @@ public Builder setExtensions(Extensions extensions) { * @since 1.21.0 */ public Args build() { - return - new Args( - defaultPort, proxyDetector, syncContext, serviceConfigParser, extensions, - scheduledExecutorService, channelLogger, executor, overrideAuthority); + return new Args(this); } } /** * Identifies an externally-defined {@link Args} extension. * - *

Uses reference equality. + *

Uses reference equality so keys should be defined as global constants. * * @param type of the value in the key-value pair */ @@ -626,15 +619,15 @@ public static Key create(String debugString) { * *

While ordinary {@link Args} should be universally useful and meaningful, extended args can * apply just to resolvers of a certain URI scheme, just to resolvers producing a particular - * type of {@link java.net.SocketAddress}, or even a individual {@link NameResolver} subclass. - * Extended args are identified by {@link Args.Key} which should be defined as a static constant - * in a java package and class appropriate to the argument's scope. + * type of {@link java.net.SocketAddress}, or even an individual {@link NameResolver} subclass. + * Extended args are identified by {@link Args.Key} which should be defined in a java package + * and class appropriate to the argument's scope. * - *

{@link Args} are normally reserved for information in support of name resolution, not the - * actual address to be resolved. However, there are rare cases where some or all of the input - * address can't be represented by any standard URI scheme or encoded as a String at all. - * Extension args can be a useful work around in these cases because they can hold an arbitrary - * Java type. + *

{@link Args} are normally reserved for information in *support* of name resolution, not + * the address to be resolved itself. However, there are rare cases where some or all of the + * input address can't be represented by any standard URI scheme or can't be encoded as a String + * at all. Extensions, in contrast, can be an arbitrary Java type making them a useful work + * around in these cases. * *

Extensions can also simply be used to avoid adding inappropriate deps to the low level * io.grpc package. From a5c714aadd568c08cfc14f8b18ad610f4fc1b725 Mon Sep 17 00:00:00 2001 From: John Cormie Date: Tue, 5 Nov 2024 13:05:29 -0800 Subject: [PATCH 08/23] javadoc --- api/src/main/java/io/grpc/NameResolver.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/api/src/main/java/io/grpc/NameResolver.java b/api/src/main/java/io/grpc/NameResolver.java index 64610968352..1fecf73918a 100644 --- a/api/src/main/java/io/grpc/NameResolver.java +++ b/api/src/main/java/io/grpc/NameResolver.java @@ -710,7 +710,7 @@ public Builder setAll(Extensions other) { } /** - * Build the extensions. + * Builds the extensions. */ public Extensions build() { if (newdata != null) { From 1874264daa50b2b071ba61e47b2e38bdbc1d5be2 Mon Sep 17 00:00:00 2001 From: John Cormie Date: Tue, 5 Nov 2024 19:33:09 -0800 Subject: [PATCH 09/23] polish --- .../java/io/grpc/ManagedChannelBuilder.java | 7 +++- api/src/main/java/io/grpc/NameResolver.java | 42 +++++++++---------- 2 files changed, 26 insertions(+), 23 deletions(-) diff --git a/api/src/main/java/io/grpc/ManagedChannelBuilder.java b/api/src/main/java/io/grpc/ManagedChannelBuilder.java index e932e0f6daf..c660b0f4f60 100644 --- a/api/src/main/java/io/grpc/ManagedChannelBuilder.java +++ b/api/src/main/java/io/grpc/ManagedChannelBuilder.java @@ -635,16 +635,19 @@ protected T addMetricSink(MetricSink metricSink) { } /** - * Provides an extended argument for the {@link NameResolver}, if applicable. + * Provides an "extended" argument for the {@link NameResolver}, if applicable, replacing any + * value previously provided for this key. * *

NB: If the selected {@link NameResolver} does not understand your extension, it will be * silently ignored. * + *

See {@link NameResolver.Args.Extensions} for more. + * * @param key identifies the argument in a type-safe manner * @param value the argument itself * @return this */ - @ExperimentalApi("https://github.com/grpc/grpc-java/issues/00000") + @ExperimentalApi("https://github.com/grpc/grpc-java/issues/1770") public T setNameResolverArg(NameResolver.Args.Key key, X value) { throw new UnsupportedOperationException(); } diff --git a/api/src/main/java/io/grpc/NameResolver.java b/api/src/main/java/io/grpc/NameResolver.java index 1fecf73918a..4c84ce3b675 100644 --- a/api/src/main/java/io/grpc/NameResolver.java +++ b/api/src/main/java/io/grpc/NameResolver.java @@ -582,11 +582,11 @@ public Args build() { } /** - * Identifies an externally-defined {@link Args} extension. + * Identifies an externally-defined extension argument that can be stored in {@link Args}. * *

Uses reference equality so keys should be defined as global constants. * - * @param type of the value in the key-value pair + * @param type of values that can be stored under this key */ @Immutable @SuppressWarnings("UnusedTypeParameter") @@ -603,11 +603,11 @@ public String toString() { } /** - * Factory method for creating instances of {@link Key}. + * Creates a new instance of {@link Key}. * * @param debugString a string used to describe the key, used for debugging. * @param Key type - * @return Key object + * @return a new instance of Key */ public static Key create(String debugString) { return new Key<>(debugString); @@ -617,24 +617,24 @@ public static Key create(String debugString) { /** * An immutable type-safe container of externally-defined {@link NameResolver} arguments. * - *

While ordinary {@link Args} should be universally useful and meaningful, extended args can - * apply just to resolvers of a certain URI scheme, just to resolvers producing a particular - * type of {@link java.net.SocketAddress}, or even an individual {@link NameResolver} subclass. - * Extended args are identified by {@link Args.Key} which should be defined in a java package - * and class appropriate to the argument's scope. + *

While ordinary {@link Args} should be universally useful and meaningful, argument {@link + * Extensions} can apply just to resolvers of a certain URI scheme, just to resolvers producing + * a particular type of {@link java.net.SocketAddress}, or even an individual {@link + * NameResolver} subclass. Extended args are identified by an instance of {@link Args.Key} which + * should be defined in a java package and class appropriate to the argument's scope. * *

{@link Args} are normally reserved for information in *support* of name resolution, not - * the address to be resolved itself. However, there are rare cases where some or all of the - * input address can't be represented by any standard URI scheme or can't be encoded as a String - * at all. Extensions, in contrast, can be an arbitrary Java type making them a useful work - * around in these cases. + * the name to be resolved itself. However, there are rare cases where all or part of the target + * name can't be represented by any standard URI scheme or can't be encoded as a String at all. + * Extensions, in contrast, can be an arbitrary Java type, making them a useful work around in + * these cases. * - *

Extensions can also simply be used to avoid adding inappropriate deps to the low level + *

Extensions can also be used simply to avoid adding inappropriate deps to the low level * io.grpc package. * *

NB: This class overrides neither {@code equals()} nor {@code hashCode()}. */ - @ExperimentalApi("https://github.com/grpc/grpc-java/issues/0000") + @ExperimentalApi("https://github.com/grpc/grpc-java/issues/1770") @Immutable public static final class Extensions { private static final IdentityHashMap, Object> EMPTY_MAP = new IdentityHashMap<>(); @@ -681,7 +681,7 @@ public String toString() { } /** - * Fluently builds an instance of {@link Extensions}. + * Fluently builds instances of {@link Extensions}. */ public static final class Builder { private Extensions base; @@ -699,16 +699,16 @@ private IdentityHashMap, Object> data(int size) { return newdata; } + /** + * Associates 'value' with 'key', replacing any previously associated value. + * + * @return this + */ public Builder set(Key key, T value) { data(1).put(key, value); return this; } - public Builder setAll(Extensions other) { - data(other.data.size()).putAll(other.data); - return this; - } - /** * Builds the extensions. */ From 2fbfc893cc28d82fbd6068ab3930a2abd98de36b Mon Sep 17 00:00:00 2001 From: John Cormie Date: Tue, 5 Nov 2024 19:34:07 -0800 Subject: [PATCH 10/23] value/key --- api/src/main/java/io/grpc/ManagedChannelBuilder.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/api/src/main/java/io/grpc/ManagedChannelBuilder.java b/api/src/main/java/io/grpc/ManagedChannelBuilder.java index c660b0f4f60..e8348b5d66f 100644 --- a/api/src/main/java/io/grpc/ManagedChannelBuilder.java +++ b/api/src/main/java/io/grpc/ManagedChannelBuilder.java @@ -636,7 +636,7 @@ protected T addMetricSink(MetricSink metricSink) { /** * Provides an "extended" argument for the {@link NameResolver}, if applicable, replacing any - * value previously provided for this key. + * 'value' previously provided for 'key'. * *

NB: If the selected {@link NameResolver} does not understand your extension, it will be * silently ignored. From a4b6f072cdb64cfb24dbb6c00085a4a8b62fd2b7 Mon Sep 17 00:00:00 2001 From: John Cormie Date: Tue, 5 Nov 2024 19:50:04 -0800 Subject: [PATCH 11/23] rename --- api/src/main/java/io/grpc/ManagedChannelBuilder.java | 4 ++-- .../src/main/java/io/grpc/internal/ManagedChannelImpl.java | 2 +- .../java/io/grpc/internal/ManagedChannelImplBuilder.java | 7 +++---- .../io/grpc/internal/ManagedChannelImplBuilderTest.java | 5 +++-- 4 files changed, 9 insertions(+), 9 deletions(-) diff --git a/api/src/main/java/io/grpc/ManagedChannelBuilder.java b/api/src/main/java/io/grpc/ManagedChannelBuilder.java index e8348b5d66f..eef123413a0 100644 --- a/api/src/main/java/io/grpc/ManagedChannelBuilder.java +++ b/api/src/main/java/io/grpc/ManagedChannelBuilder.java @@ -638,8 +638,8 @@ protected T addMetricSink(MetricSink metricSink) { * Provides an "extended" argument for the {@link NameResolver}, if applicable, replacing any * 'value' previously provided for 'key'. * - *

NB: If the selected {@link NameResolver} does not understand your extension, it will be - * silently ignored. + *

NB: If the selected {@link NameResolver} does not understand 'key', or target URI resolution + * isn't needed at all, your extended argument will be silently ignored. * *

See {@link NameResolver.Args.Extensions} for more. * diff --git a/core/src/main/java/io/grpc/internal/ManagedChannelImpl.java b/core/src/main/java/io/grpc/internal/ManagedChannelImpl.java index 60fba3f1383..a88b6fe8cc2 100644 --- a/core/src/main/java/io/grpc/internal/ManagedChannelImpl.java +++ b/core/src/main/java/io/grpc/internal/ManagedChannelImpl.java @@ -599,7 +599,7 @@ ClientStream newSubstream( .setChannelLogger(channelLogger) .setOffloadExecutor(this.offloadExecutorHolder) .setOverrideAuthority(this.authorityOverride) - .setExtensions(builder.resolverExArgsBuilder.build()) + .setExtensions(builder.nameResolverArgsExtBuilder.build()) .build(); this.nameResolver = getNameResolver( targetUri, authorityOverride, nameResolverProvider, nameResolverArgs); diff --git a/core/src/main/java/io/grpc/internal/ManagedChannelImplBuilder.java b/core/src/main/java/io/grpc/internal/ManagedChannelImplBuilder.java index 350b87f2599..7271c606854 100644 --- a/core/src/main/java/io/grpc/internal/ManagedChannelImplBuilder.java +++ b/core/src/main/java/io/grpc/internal/ManagedChannelImplBuilder.java @@ -151,6 +151,8 @@ public static ManagedChannelBuilder forTarget(String target) { private final List interceptors = new ArrayList<>(); NameResolverRegistry nameResolverRegistry = NameResolverRegistry.getDefaultRegistry(); + final NameResolver.Args.Extensions.Builder nameResolverArgsExtBuilder = + NameResolver.Args.Extensions.newBuilder(); final List transportFilters = new ArrayList<>(); @@ -198,9 +200,6 @@ public static ManagedChannelBuilder forTarget(String target) { @Nullable ProxyDetector proxyDetector; - NameResolver.Args.Extensions.Builder resolverExArgsBuilder = - NameResolver.Args.Extensions.newBuilder(); - private boolean authorityCheckerDisabled; private boolean statsEnabled = true; private boolean recordStartedRpcs = true; @@ -559,7 +558,7 @@ public ManagedChannelImplBuilder defaultServiceConfig(@Nullable Map s @Override public ManagedChannelImplBuilder setNameResolverArg(NameResolver.Args.Key key, X value) { - resolverExArgsBuilder.set(key, value); + nameResolverArgsExtBuilder.set(key, value); return this; } diff --git a/core/src/test/java/io/grpc/internal/ManagedChannelImplBuilderTest.java b/core/src/test/java/io/grpc/internal/ManagedChannelImplBuilderTest.java index 72eb57cc142..9c486bff1a1 100644 --- a/core/src/test/java/io/grpc/internal/ManagedChannelImplBuilderTest.java +++ b/core/src/test/java/io/grpc/internal/ManagedChannelImplBuilderTest.java @@ -766,11 +766,12 @@ public void disableNameResolverServiceConfig() { @Test public void setNameResolverExtArgs() { - assertThat(builder.resolverExArgsBuilder.build()) + assertThat(builder.nameResolverArgsExtBuilder.build()) .isSameInstanceAs(NameResolver.Args.Extensions.EMPTY); + NameResolver.Args.Key testKey = NameResolver.Args.Key.create("test-key"); builder.setNameResolverArg(testKey, 42); - assertThat(builder.resolverExArgsBuilder.build().get(testKey)).isEqualTo(42); + assertThat(builder.nameResolverArgsExtBuilder.build().get(testKey)).isEqualTo(42); } @Test From 7f48a33bfafde490d6c3677cd06f674c9385092e Mon Sep 17 00:00:00 2001 From: John Cormie Date: Tue, 19 Nov 2024 18:43:13 -0800 Subject: [PATCH 12/23] Address code review comments. Fully remove Args.Extensions from the public API (@Internal) Make TODO non-javadoc google-java-format unused imports --- .../io/grpc/ForwardingChannelBuilder2.java | 1 - .../java/io/grpc/ManagedChannelBuilder.java | 3 +- api/src/main/java/io/grpc/NameResolver.java | 88 ++++++++++--------- .../io/grpc/internal/ManagedChannelImpl.java | 2 +- 4 files changed, 47 insertions(+), 47 deletions(-) diff --git a/api/src/main/java/io/grpc/ForwardingChannelBuilder2.java b/api/src/main/java/io/grpc/ForwardingChannelBuilder2.java index fca87e54090..78fe730d91a 100644 --- a/api/src/main/java/io/grpc/ForwardingChannelBuilder2.java +++ b/api/src/main/java/io/grpc/ForwardingChannelBuilder2.java @@ -18,7 +18,6 @@ import com.google.common.base.MoreObjects; import com.google.errorprone.annotations.DoNotCall; -import io.grpc.NameResolver; import java.util.List; import java.util.Map; import java.util.concurrent.Executor; diff --git a/api/src/main/java/io/grpc/ManagedChannelBuilder.java b/api/src/main/java/io/grpc/ManagedChannelBuilder.java index eef123413a0..0d086602aac 100644 --- a/api/src/main/java/io/grpc/ManagedChannelBuilder.java +++ b/api/src/main/java/io/grpc/ManagedChannelBuilder.java @@ -17,7 +17,6 @@ package io.grpc; import com.google.common.base.Preconditions; -import io.grpc.NameResolver; import java.util.List; import java.util.Map; import java.util.concurrent.Executor; @@ -641,7 +640,7 @@ protected T addMetricSink(MetricSink metricSink) { *

NB: If the selected {@link NameResolver} does not understand 'key', or target URI resolution * isn't needed at all, your extended argument will be silently ignored. * - *

See {@link NameResolver.Args.Extensions} for more. + *

See {@link NameResolver.Args#getExtension(NameResolver.Args.Key)} for more. * * @param key identifies the argument in a type-safe manner * @param value the argument itself diff --git a/api/src/main/java/io/grpc/NameResolver.java b/api/src/main/java/io/grpc/NameResolver.java index 4c84ce3b675..6c9b1a96059 100644 --- a/api/src/main/java/io/grpc/NameResolver.java +++ b/api/src/main/java/io/grpc/NameResolver.java @@ -279,7 +279,7 @@ public Status onResult2(ResolutionResult resolutionResult) { *

Args applicable to all {@link NameResolver}s are defined here using ordinary setters and * getters. This container can also hold externally-defined "extended" args that aren't so widely * useful or that would be inappropriate dependencies for this low level API. See {@link - * Args.Extensions} for more. + * Args#getExtension} for more. * *

Note this class overrides neither {@code equals()} nor {@code hashCode()}. * @@ -303,7 +303,8 @@ private Args(Builder builder) { this.syncContext = checkNotNull(builder.syncContext, "syncContext not set"); this.serviceConfigParser = checkNotNull(builder.serviceConfigParser, "serviceConfigParser not set"); - this.extensions = checkNotNull(builder.extensions, "extensions not set"); + this.extensions = + builder.extensionsBuilder != null ? builder.extensionsBuilder.build() : Extensions.EMPTY; this.scheduledExecutorService = builder.scheduledExecutorService; this.channelLogger = builder.channelLogger; this.executor = builder.executor; @@ -314,10 +315,9 @@ private Args(Builder builder) { * The port number used in case the target or the underlying naming system doesn't provide a * port number. * - *

TODO: Only meaningful for InetSocketAddress producers. Move to {@link Extensions}? - * * @since 1.21.0 */ + //

TODO: Only meaningful for InetSocketAddress producers. Move to {@link Extensions}? public int getDefaultPort() { return defaultPort; } @@ -371,9 +371,23 @@ public ServiceConfigParser getServiceConfigParser() { } /** - * Gets the value of an extended arg by key, or {@code null} if it's not set. + * Gets the value of an "extension" arg by key, or {@code null} if it's not set. + * + *

While ordinary {@link Args} should be universally useful and meaningful, extension + * arguments can apply just to resolvers of a certain URI scheme, just to resolvers producing + * a particular type of {@link java.net.SocketAddress}, or even an individual {@link + * NameResolver} subclass. Extension args are identified by an instance of {@link Args.Key} + * which should be defined in a java package and class appropriate to the argument's scope. + * + *

{@link Args} are normally reserved for information in *support* of name resolution, not + * the name to be resolved itself. However, there are rare cases where all or part of the target + * name can't be represented by any standard URI scheme or can't be encoded as a String at all. + * Extension args, in contrast, can be an arbitrary Java type, making them a useful work around + * in these cases. + * + *

Extension args can also be used simply to avoid adding inappropriate deps to the low level + * io.grpc package. */ - @SuppressWarnings("unchecked") @Nullable public T getExtension(Key key) { return extensions.get(key); @@ -443,7 +457,7 @@ public Builder toBuilder() { builder.setProxyDetector(proxyDetector); builder.setSynchronizationContext(syncContext); builder.setServiceConfigParser(serviceConfigParser); - builder.setExtensions(extensions); + builder.extensionsBuilder = extensions.toBuilder(); builder.setScheduledExecutorService(scheduledExecutorService); builder.setChannelLogger(channelLogger); builder.setOffloadExecutor(executor); @@ -474,7 +488,7 @@ public static final class Builder { private ChannelLogger channelLogger; private Executor executor; private String overrideAuthority; - private Extensions extensions = Extensions.EMPTY; + private Extensions.Builder extensionsBuilder = Extensions.newBuilder(); Builder() { } @@ -563,11 +577,18 @@ public Builder setOverrideAuthority(String authority) { /** * See {@link Args#getExtension(Key)}. - * - *

Optional, {@link Extensions#EMPTY} by default. */ - public Builder setExtensions(Extensions extensions) { - this.extensions = extensions; + public Builder setExtension(Key key, T value) { + extensionsBuilder.set(key, value); + return this; + } + + /** + * Copies each extension argument from 'extensions' into this Builder. + */ + @Internal + public Builder setAllExtensions(Extensions extensions) { + extensionsBuilder.setAll(extensions); return this; } @@ -617,31 +638,14 @@ public static Key create(String debugString) { /** * An immutable type-safe container of externally-defined {@link NameResolver} arguments. * - *

While ordinary {@link Args} should be universally useful and meaningful, argument {@link - * Extensions} can apply just to resolvers of a certain URI scheme, just to resolvers producing - * a particular type of {@link java.net.SocketAddress}, or even an individual {@link - * NameResolver} subclass. Extended args are identified by an instance of {@link Args.Key} which - * should be defined in a java package and class appropriate to the argument's scope. - * - *

{@link Args} are normally reserved for information in *support* of name resolution, not - * the name to be resolved itself. However, there are rare cases where all or part of the target - * name can't be represented by any standard URI scheme or can't be encoded as a String at all. - * Extensions, in contrast, can be an arbitrary Java type, making them a useful work around in - * these cases. - * - *

Extensions can also be used simply to avoid adding inappropriate deps to the low level - * io.grpc package. - * *

NB: This class overrides neither {@code equals()} nor {@code hashCode()}. */ - @ExperimentalApi("https://github.com/grpc/grpc-java/issues/1770") @Immutable + @Internal public static final class Extensions { private static final IdentityHashMap, Object> EMPTY_MAP = new IdentityHashMap<>(); - /** - * The canonical empty instance. - */ + /** The canonical empty instance. */ public static final Extensions EMPTY = new Extensions(EMPTY_MAP); private final IdentityHashMap, Object> data; @@ -651,24 +655,21 @@ private Extensions(IdentityHashMap, Object> data) { this.data = data; } - /** - * Gets the value for the key, or {@code null} if it's not present. - */ + /** Gets the value for the key, or {@code null} if it's not present. */ @SuppressWarnings("unchecked") @Nullable public T get(Key key) { return (T) data.get(key); } - /** - * Creates a new builder. - */ + /** Creates a new builder. */ public static Builder newBuilder() { return new Builder(EMPTY); } /** * Creates a new builder that is pre-populated with the content of this container. + * * @return a new builder. */ public Builder toBuilder() { @@ -680,9 +681,7 @@ public String toString() { return data.toString(); } - /** - * Fluently builds instances of {@link Extensions}. - */ + /** Fluently builds instances of {@link Extensions}. */ public static final class Builder { private Extensions base; private IdentityHashMap, Object> newdata; @@ -709,9 +708,12 @@ public Builder set(Key key, T value) { return this; } - /** - * Builds the extensions. - */ + public Builder setAll(Extensions other) { + data(other.data.size()).putAll(other.data); + return this; + } + + /** Builds the extensions. */ public Extensions build() { if (newdata != null) { for (Map.Entry, Object> entry : base.data.entrySet()) { diff --git a/core/src/main/java/io/grpc/internal/ManagedChannelImpl.java b/core/src/main/java/io/grpc/internal/ManagedChannelImpl.java index a88b6fe8cc2..3e96b54c98e 100644 --- a/core/src/main/java/io/grpc/internal/ManagedChannelImpl.java +++ b/core/src/main/java/io/grpc/internal/ManagedChannelImpl.java @@ -599,7 +599,7 @@ ClientStream newSubstream( .setChannelLogger(channelLogger) .setOffloadExecutor(this.offloadExecutorHolder) .setOverrideAuthority(this.authorityOverride) - .setExtensions(builder.nameResolverArgsExtBuilder.build()) + .setAllExtensions(builder.nameResolverArgsExtBuilder.build()) .build(); this.nameResolver = getNameResolver( targetUri, authorityOverride, nameResolverProvider, nameResolverArgs); From 76d1742769ddd960c6875b18c336e6997a086385 Mon Sep 17 00:00:00 2001 From: John Cormie Date: Tue, 19 Nov 2024 18:45:33 -0800 Subject: [PATCH 13/23] javadoc --- api/src/main/java/io/grpc/NameResolver.java | 3 +++ 1 file changed, 3 insertions(+) diff --git a/api/src/main/java/io/grpc/NameResolver.java b/api/src/main/java/io/grpc/NameResolver.java index 6c9b1a96059..f0149f42f67 100644 --- a/api/src/main/java/io/grpc/NameResolver.java +++ b/api/src/main/java/io/grpc/NameResolver.java @@ -708,6 +708,9 @@ public Builder set(Key key, T value) { return this; } + /** + * Copies all entries from 'other' into this Builder. + */ public Builder setAll(Extensions other) { data(other.data.size()).putAll(other.data); return this; From a216397a03c2c1513d66ca3b07f26385f04bc0ea Mon Sep 17 00:00:00 2001 From: John Cormie Date: Tue, 19 Nov 2024 19:23:55 -0800 Subject: [PATCH 14/23] Copy forward AttributesTest.java --- api/src/main/java/io/grpc/NameResolver.java | 25 ++++--- .../grpc/NameResolverArgsExtensionsTest.java | 67 +++++++++++++++++++ 2 files changed, 79 insertions(+), 13 deletions(-) create mode 100644 api/src/test/java/io/grpc/NameResolverArgsExtensionsTest.java diff --git a/api/src/main/java/io/grpc/NameResolver.java b/api/src/main/java/io/grpc/NameResolver.java index f0149f42f67..cb584b1b5a1 100644 --- a/api/src/main/java/io/grpc/NameResolver.java +++ b/api/src/main/java/io/grpc/NameResolver.java @@ -31,6 +31,7 @@ import java.util.IdentityHashMap; import java.util.List; import java.util.Map; +import java.util.Set; import java.util.concurrent.Executor; import java.util.concurrent.ScheduledExecutorService; import javax.annotation.Nullable; @@ -374,10 +375,10 @@ public ServiceConfigParser getServiceConfigParser() { * Gets the value of an "extension" arg by key, or {@code null} if it's not set. * *

While ordinary {@link Args} should be universally useful and meaningful, extension - * arguments can apply just to resolvers of a certain URI scheme, just to resolvers producing - * a particular type of {@link java.net.SocketAddress}, or even an individual {@link - * NameResolver} subclass. Extension args are identified by an instance of {@link Args.Key} - * which should be defined in a java package and class appropriate to the argument's scope. + * arguments can apply just to resolvers of a certain URI scheme, just to resolvers producing a + * particular type of {@link java.net.SocketAddress}, or even an individual {@link NameResolver} + * subclass. Extension args are identified by an instance of {@link Args.Key} which should be + * defined in a java package and class appropriate to the argument's scope. * *

{@link Args} are normally reserved for information in *support* of name resolution, not * the name to be resolved itself. However, there are rare cases where all or part of the target @@ -575,17 +576,13 @@ public Builder setOverrideAuthority(String authority) { return this; } - /** - * See {@link Args#getExtension(Key)}. - */ + /** See {@link Args#getExtension(Key)}. */ public Builder setExtension(Key key, T value) { extensionsBuilder.set(key, value); return this; } - /** - * Copies each extension argument from 'extensions' into this Builder. - */ + /** Copies each extension argument from 'extensions' into this Builder. */ @Internal public Builder setAllExtensions(Extensions extensions) { extensionsBuilder.setAll(extensions); @@ -662,6 +659,10 @@ public T get(Key key) { return (T) data.get(key); } + Set> keysForTest() { + return Collections.unmodifiableSet(data.keySet()); + } + /** Creates a new builder. */ public static Builder newBuilder() { return new Builder(EMPTY); @@ -708,9 +709,7 @@ public Builder set(Key key, T value) { return this; } - /** - * Copies all entries from 'other' into this Builder. - */ + /** Copies all entries from 'other' into this Builder. */ public Builder setAll(Extensions other) { data(other.data.size()).putAll(other.data); return this; diff --git a/api/src/test/java/io/grpc/NameResolverArgsExtensionsTest.java b/api/src/test/java/io/grpc/NameResolverArgsExtensionsTest.java new file mode 100644 index 00000000000..7bb23ff7163 --- /dev/null +++ b/api/src/test/java/io/grpc/NameResolverArgsExtensionsTest.java @@ -0,0 +1,67 @@ +/* + * Copyright 2024 The gRPC Authors + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package io.grpc; + +import static com.google.common.truth.Truth.assertThat; +import static org.junit.Assert.assertSame; + +import io.grpc.NameResolver.Args; +import io.grpc.NameResolver.Args.Extensions; +import org.junit.Test; +import org.junit.runner.RunWith; +import org.junit.runners.JUnit4; + +/** Unit tests for {@link NameResolver.Args.Extensions}. */ +@RunWith(JUnit4.class) +public class NameResolverArgsExtensionsTest { + private static final Args.Key YOLO_KEY = Args.Key.create("yolo"); + + @Test + public void buildExtensions() { + Extensions attrs = Extensions.newBuilder().set(YOLO_KEY, "To be, or not to be?").build(); + assertSame("To be, or not to be?", attrs.get(YOLO_KEY)); + assertThat(attrs.keysForTest()).hasSize(1); + } + + @Test + public void duplicates() { + Extensions attrs = + Extensions.newBuilder() + .set(YOLO_KEY, "To be?") + .set(YOLO_KEY, "Or not to be?") + .set(Args.Key.create("yolo"), "I'm not a duplicate") + .build(); + assertThat(attrs.get(YOLO_KEY)).isEqualTo("Or not to be?"); + assertThat(attrs.keysForTest()).hasSize(2); + } + + @Test + public void toBuilder() { + Extensions attrs = + Extensions.newBuilder().set(YOLO_KEY, "To be?").build().toBuilder() + .set(YOLO_KEY, "Or not to be?") + .set(Args.Key.create("yolo"), "I'm not a duplicate") + .build(); + assertThat(attrs.get(YOLO_KEY)).isEqualTo("Or not to be?"); + assertThat(attrs.keysForTest()).hasSize(2); + } + + @Test + public void empty() { + assertThat(Extensions.EMPTY.keysForTest()).isEmpty(); + } +} From 29d01c79a53d6a03f91a8c85d2741ae7b30afe58 Mon Sep 17 00:00:00 2001 From: John Cormie Date: Wed, 20 Nov 2024 10:51:17 -0800 Subject: [PATCH 15/23] Beef up unit tests Update NameResolverTest. Include setAll() coverage --- .../grpc/NameResolverArgsExtensionsTest.java | 48 +++++++++++++------ .../test/java/io/grpc/NameResolverTest.java | 12 +++-- 2 files changed, 43 insertions(+), 17 deletions(-) diff --git a/api/src/test/java/io/grpc/NameResolverArgsExtensionsTest.java b/api/src/test/java/io/grpc/NameResolverArgsExtensionsTest.java index 7bb23ff7163..56001c93c34 100644 --- a/api/src/test/java/io/grpc/NameResolverArgsExtensionsTest.java +++ b/api/src/test/java/io/grpc/NameResolverArgsExtensionsTest.java @@ -28,40 +28,60 @@ /** Unit tests for {@link NameResolver.Args.Extensions}. */ @RunWith(JUnit4.class) public class NameResolverArgsExtensionsTest { - private static final Args.Key YOLO_KEY = Args.Key.create("yolo"); + private static final Args.Key FOO_KEY = Args.Key.create("foo"); + private static final Args.Key BAR_KEY = Args.Key.create("bar"); + private static final Args.Key QUX_KEY = Args.Key.create("qux"); @Test public void buildExtensions() { - Extensions attrs = Extensions.newBuilder().set(YOLO_KEY, "To be, or not to be?").build(); - assertSame("To be, or not to be?", attrs.get(YOLO_KEY)); - assertThat(attrs.keysForTest()).hasSize(1); + Extensions exts = Extensions.newBuilder().set(FOO_KEY, "To be, or not to be?").build(); + assertSame("To be, or not to be?", exts.get(FOO_KEY)); + assertThat(exts.keysForTest()).hasSize(1); } @Test public void duplicates() { - Extensions attrs = + Extensions exts = Extensions.newBuilder() - .set(YOLO_KEY, "To be?") - .set(YOLO_KEY, "Or not to be?") + .set(FOO_KEY, "To be?") + .set(FOO_KEY, "Or not to be?") .set(Args.Key.create("yolo"), "I'm not a duplicate") .build(); - assertThat(attrs.get(YOLO_KEY)).isEqualTo("Or not to be?"); - assertThat(attrs.keysForTest()).hasSize(2); + assertThat(exts.get(FOO_KEY)).isEqualTo("Or not to be?"); + assertThat(exts.keysForTest()).hasSize(2); } @Test public void toBuilder() { - Extensions attrs = - Extensions.newBuilder().set(YOLO_KEY, "To be?").build().toBuilder() - .set(YOLO_KEY, "Or not to be?") + Extensions exts = + Extensions.newBuilder().set(FOO_KEY, "To be?").build().toBuilder() + .set(FOO_KEY, "Or not to be?") .set(Args.Key.create("yolo"), "I'm not a duplicate") .build(); - assertThat(attrs.get(YOLO_KEY)).isEqualTo("Or not to be?"); - assertThat(attrs.keysForTest()).hasSize(2); + assertThat(exts.get(FOO_KEY)).isEqualTo("Or not to be?"); + assertThat(exts.keysForTest()).hasSize(2); } @Test public void empty() { assertThat(Extensions.EMPTY.keysForTest()).isEmpty(); } + + @Test + public void setAll() { + Extensions newExts = + Extensions.newBuilder() + .set(FOO_KEY, "foo-orig") + .set(BAR_KEY, "bar-orig") + .setAll( + Extensions.newBuilder() + .set(FOO_KEY, "foo-updated") + .set(QUX_KEY, "qux-updated") + .build()) + .build(); + assertThat(newExts.get(FOO_KEY)).isEqualTo("foo-updated"); + assertThat(newExts.get(BAR_KEY)).isEqualTo("bar-orig"); + assertThat(newExts.get(QUX_KEY)).isEqualTo("qux-updated"); + assertThat(newExts.keysForTest()).hasSize(3); + } } diff --git a/api/src/test/java/io/grpc/NameResolverTest.java b/api/src/test/java/io/grpc/NameResolverTest.java index 1bc32ee7b1d..223ba651340 100644 --- a/api/src/test/java/io/grpc/NameResolverTest.java +++ b/api/src/test/java/io/grpc/NameResolverTest.java @@ -47,9 +47,11 @@ public class NameResolverTest { private static final List ADDRESSES = Collections.singletonList( new EquivalentAddressGroup(new FakeSocketAddress("fake-address-1"), Attributes.EMPTY)); - private static final Attributes.Key YOLO_KEY = Attributes.Key.create("yolo"); - private static Attributes ATTRIBUTES = Attributes.newBuilder() - .set(YOLO_KEY, "To be, or not to be?").build(); + private static final Attributes.Key YOLO_ATTR_KEY = Attributes.Key.create("yolo"); + private static Attributes ATTRIBUTES = + Attributes.newBuilder().set(YOLO_ATTR_KEY, "To be, or not to be?").build(); + private static final NameResolver.Args.Key EXT_ARG_KEY = + NameResolver.Args.Key.create("foo"); private static ConfigOrError CONFIG = ConfigOrError.fromConfig("foo"); @Rule @@ -64,6 +66,7 @@ public class NameResolverTest { private final ChannelLogger channelLogger = mock(ChannelLogger.class); private final Executor executor = Executors.newSingleThreadExecutor(); private final String overrideAuthority = "grpc.io"; + private final int extensionArgValue = 42; @Mock NameResolver.Listener mockListener; @Test @@ -77,6 +80,7 @@ public void args() { assertThat(args.getChannelLogger()).isSameInstanceAs(channelLogger); assertThat(args.getOffloadExecutor()).isSameInstanceAs(executor); assertThat(args.getOverrideAuthority()).isSameInstanceAs(overrideAuthority); + assertThat(args.getExtension(EXT_ARG_KEY)).isEqualTo(extensionArgValue); NameResolver.Args args2 = args.toBuilder().build(); assertThat(args2.getDefaultPort()).isEqualTo(defaultPort); @@ -87,6 +91,7 @@ public void args() { assertThat(args2.getChannelLogger()).isSameInstanceAs(channelLogger); assertThat(args2.getOffloadExecutor()).isSameInstanceAs(executor); assertThat(args2.getOverrideAuthority()).isSameInstanceAs(overrideAuthority); + assertThat(args.getExtension(EXT_ARG_KEY)).isEqualTo(extensionArgValue); assertThat(args2).isNotSameInstanceAs(args); assertThat(args2).isNotEqualTo(args); @@ -102,6 +107,7 @@ private NameResolver.Args createArgs() { .setChannelLogger(channelLogger) .setOffloadExecutor(executor) .setOverrideAuthority(overrideAuthority) + .setExtension(EXT_ARG_KEY, extensionArgValue) .build(); } From 9ff19f7d451ffb3b2f39ca6e3fd9048171c7446e Mon Sep 17 00:00:00 2001 From: John Cormie Date: Mon, 9 Dec 2024 17:59:38 -0800 Subject: [PATCH 16/23] Rename get/setExtension to be like CallOptions#get/setOption --- api/src/main/java/io/grpc/ManagedChannelBuilder.java | 2 +- api/src/main/java/io/grpc/NameResolver.java | 8 ++++---- api/src/test/java/io/grpc/NameResolverTest.java | 6 +++--- .../java/io/grpc/internal/ManagedChannelImplTest.java | 2 +- 4 files changed, 9 insertions(+), 9 deletions(-) diff --git a/api/src/main/java/io/grpc/ManagedChannelBuilder.java b/api/src/main/java/io/grpc/ManagedChannelBuilder.java index 0d086602aac..fe2e8949a56 100644 --- a/api/src/main/java/io/grpc/ManagedChannelBuilder.java +++ b/api/src/main/java/io/grpc/ManagedChannelBuilder.java @@ -640,7 +640,7 @@ protected T addMetricSink(MetricSink metricSink) { *

NB: If the selected {@link NameResolver} does not understand 'key', or target URI resolution * isn't needed at all, your extended argument will be silently ignored. * - *

See {@link NameResolver.Args#getExtension(NameResolver.Args.Key)} for more. + *

See {@link NameResolver.Args#getArg(NameResolver.Args.Key)} for more. * * @param key identifies the argument in a type-safe manner * @param value the argument itself diff --git a/api/src/main/java/io/grpc/NameResolver.java b/api/src/main/java/io/grpc/NameResolver.java index 6565655cf2f..f685ae58e6c 100644 --- a/api/src/main/java/io/grpc/NameResolver.java +++ b/api/src/main/java/io/grpc/NameResolver.java @@ -282,7 +282,7 @@ public Status onResult2(ResolutionResult resolutionResult) { *

Args applicable to all {@link NameResolver}s are defined here using ordinary setters and * getters. This container can also hold externally-defined "extended" args that aren't so widely * useful or that would be inappropriate dependencies for this low level API. See {@link - * Args#getExtension} for more. + * Args#getArg} for more. * *

Note this class overrides neither {@code equals()} nor {@code hashCode()}. * @@ -394,7 +394,7 @@ public ServiceConfigParser getServiceConfigParser() { * io.grpc package. */ @Nullable - public T getExtension(Key key) { + public T getArg(Key key) { return extensions.get(key); } @@ -591,8 +591,8 @@ public Builder setOverrideAuthority(String authority) { return this; } - /** See {@link Args#getExtension(Key)}. */ - public Builder setExtension(Key key, T value) { + /** See {@link Args#getArg(Key)}. */ + public Builder setArg(Key key, T value) { extensionsBuilder.set(key, value); return this; } diff --git a/api/src/test/java/io/grpc/NameResolverTest.java b/api/src/test/java/io/grpc/NameResolverTest.java index 97647c7df91..d104b89de9f 100644 --- a/api/src/test/java/io/grpc/NameResolverTest.java +++ b/api/src/test/java/io/grpc/NameResolverTest.java @@ -82,7 +82,7 @@ public void args() { assertThat(args.getOffloadExecutor()).isSameInstanceAs(executor); assertThat(args.getOverrideAuthority()).isSameInstanceAs(overrideAuthority); assertThat(args.getMetricRecorder()).isSameInstanceAs(metricRecorder); - assertThat(args.getExtension(EXT_ARG_KEY)).isEqualTo(extensionArgValue); + assertThat(args.getArg(EXT_ARG_KEY)).isEqualTo(extensionArgValue); NameResolver.Args args2 = args.toBuilder().build(); assertThat(args2.getDefaultPort()).isEqualTo(defaultPort); @@ -94,7 +94,7 @@ public void args() { assertThat(args2.getOffloadExecutor()).isSameInstanceAs(executor); assertThat(args2.getOverrideAuthority()).isSameInstanceAs(overrideAuthority); assertThat(args.getMetricRecorder()).isSameInstanceAs(metricRecorder); - assertThat(args.getExtension(EXT_ARG_KEY)).isEqualTo(extensionArgValue); + assertThat(args.getArg(EXT_ARG_KEY)).isEqualTo(extensionArgValue); assertThat(args2).isNotSameInstanceAs(args); assertThat(args2).isNotEqualTo(args); @@ -111,7 +111,7 @@ private NameResolver.Args createArgs() { .setOffloadExecutor(executor) .setOverrideAuthority(overrideAuthority) .setMetricRecorder(metricRecorder) - .setExtension(EXT_ARG_KEY, extensionArgValue) + .setArg(EXT_ARG_KEY, extensionArgValue) .build(); } diff --git a/core/src/test/java/io/grpc/internal/ManagedChannelImplTest.java b/core/src/test/java/io/grpc/internal/ManagedChannelImplTest.java index 6a145b72851..e2ebadfe907 100644 --- a/core/src/test/java/io/grpc/internal/ManagedChannelImplTest.java +++ b/core/src/test/java/io/grpc/internal/ManagedChannelImplTest.java @@ -4281,7 +4281,7 @@ public String getDefaultScheme() { assertThat(args).isNotNull(); assertThat(args.getDefaultPort()).isEqualTo(DEFAULT_PORT); assertThat(args.getProxyDetector()).isSameInstanceAs(neverProxy); - assertThat(args.getExtension(TEST_RESOLVER_ARG_EXT_KEY)).isEqualTo("test-value"); + assertThat(args.getArg(TEST_RESOLVER_ARG_EXT_KEY)).isEqualTo("test-value"); verify(offloadExecutor, never()).execute(any(Runnable.class)); args.getOffloadExecutor() From 3124a9169bc02bd5d28065995bcf9e719a98a84b Mon Sep 17 00:00:00 2001 From: John Cormie Date: Tue, 17 Dec 2024 15:32:55 -0800 Subject: [PATCH 17/23] Delete Extensions nested class completely. Call these "custom" args rather than extensions for consistency with CallOptions. --- api/src/main/java/io/grpc/NameResolver.java | 132 +++--------------- .../grpc/NameResolverArgsExtensionsTest.java | 87 ------------ .../test/java/io/grpc/NameResolverTest.java | 12 +- .../io/grpc/internal/ManagedChannelImpl.java | 9 +- .../internal/ManagedChannelImplBuilder.java | 30 ++-- .../ManagedChannelImplBuilderTest.java | 6 +- .../grpc/internal/ManagedChannelImplTest.java | 6 +- 7 files changed, 57 insertions(+), 225 deletions(-) delete mode 100644 api/src/test/java/io/grpc/NameResolverArgsExtensionsTest.java diff --git a/api/src/main/java/io/grpc/NameResolver.java b/api/src/main/java/io/grpc/NameResolver.java index f685ae58e6c..625495f4347 100644 --- a/api/src/main/java/io/grpc/NameResolver.java +++ b/api/src/main/java/io/grpc/NameResolver.java @@ -31,7 +31,6 @@ import java.util.IdentityHashMap; import java.util.List; import java.util.Map; -import java.util.Set; import java.util.concurrent.Executor; import java.util.concurrent.ScheduledExecutorService; import javax.annotation.Nullable; @@ -294,12 +293,12 @@ public static final class Args { private final ProxyDetector proxyDetector; private final SynchronizationContext syncContext; private final ServiceConfigParser serviceConfigParser; - private final Extensions extensions; @Nullable private final ScheduledExecutorService scheduledExecutorService; @Nullable private final ChannelLogger channelLogger; @Nullable private final Executor executor; @Nullable private final String overrideAuthority; @Nullable private final MetricRecorder metricRecorder; + @Nullable private final IdentityHashMap, Object> customArgs; private Args(Builder builder) { this.defaultPort = checkNotNull(builder.defaultPort, "defaultPort not set"); @@ -307,13 +306,12 @@ private Args(Builder builder) { this.syncContext = checkNotNull(builder.syncContext, "syncContext not set"); this.serviceConfigParser = checkNotNull(builder.serviceConfigParser, "serviceConfigParser not set"); - this.extensions = - builder.extensionsBuilder != null ? builder.extensionsBuilder.build() : Extensions.EMPTY; this.scheduledExecutorService = builder.scheduledExecutorService; this.channelLogger = builder.channelLogger; this.executor = builder.executor; this.overrideAuthority = builder.overrideAuthority; this.metricRecorder = builder.metricRecorder; + this.customArgs = cloneCustomArgs(builder.customArgs); } /** @@ -393,9 +391,10 @@ public ServiceConfigParser getServiceConfigParser() { *

Extension args can also be used simply to avoid adding inappropriate deps to the low level * io.grpc package. */ + @SuppressWarnings("unchecked") // Cast is safe because all put()s go through the setArg() API. @Nullable public T getArg(Key key) { - return extensions.get(key); + return customArgs != null ? (T) customArgs.get(key) : null; } /** @@ -451,7 +450,7 @@ public String toString() { .add("proxyDetector", proxyDetector) .add("syncContext", syncContext) .add("serviceConfigParser", serviceConfigParser) - .add("extensions", extensions) + .add("customArgs", customArgs) .add("scheduledExecutorService", scheduledExecutorService) .add("channelLogger", channelLogger) .add("executor", executor) @@ -471,12 +470,12 @@ public Builder toBuilder() { builder.setProxyDetector(proxyDetector); builder.setSynchronizationContext(syncContext); builder.setServiceConfigParser(serviceConfigParser); - builder.extensionsBuilder = extensions.toBuilder(); builder.setScheduledExecutorService(scheduledExecutorService); builder.setChannelLogger(channelLogger); builder.setOffloadExecutor(executor); builder.setOverrideAuthority(overrideAuthority); builder.setMetricRecorder(metricRecorder); + builder.customArgs = cloneCustomArgs(customArgs); return builder; } @@ -504,7 +503,7 @@ public static final class Builder { private Executor executor; private String overrideAuthority; private MetricRecorder metricRecorder; - private Extensions.Builder extensionsBuilder = Extensions.newBuilder(); + private IdentityHashMap, Object> customArgs; Builder() { } @@ -593,14 +592,10 @@ public Builder setOverrideAuthority(String authority) { /** See {@link Args#getArg(Key)}. */ public Builder setArg(Key key, T value) { - extensionsBuilder.set(key, value); - return this; - } - - /** Copies each extension argument from 'extensions' into this Builder. */ - @Internal - public Builder setAllExtensions(Extensions extensions) { - extensionsBuilder.setAll(extensions); + if (customArgs == null) { + customArgs = new IdentityHashMap<>(); + } + customArgs.put(key, value); return this; } @@ -654,105 +649,6 @@ public static Key create(String debugString) { return new Key<>(debugString); } } - - /** - * An immutable type-safe container of externally-defined {@link NameResolver} arguments. - * - *

NB: This class overrides neither {@code equals()} nor {@code hashCode()}. - */ - @Immutable - @Internal - public static final class Extensions { - private static final IdentityHashMap, Object> EMPTY_MAP = new IdentityHashMap<>(); - - /** The canonical empty instance. */ - public static final Extensions EMPTY = new Extensions(EMPTY_MAP); - - private final IdentityHashMap, Object> data; - - private Extensions(IdentityHashMap, Object> data) { - assert data != null; - this.data = data; - } - - /** Gets the value for the key, or {@code null} if it's not present. */ - @SuppressWarnings("unchecked") - @Nullable - public T get(Key key) { - return (T) data.get(key); - } - - Set> keysForTest() { - return Collections.unmodifiableSet(data.keySet()); - } - - /** Creates a new builder. */ - public static Builder newBuilder() { - return new Builder(EMPTY); - } - - /** - * Creates a new builder that is pre-populated with the content of this container. - * - * @return a new builder. - */ - public Builder toBuilder() { - return new Builder(this); - } - - @Override - public String toString() { - return data.toString(); - } - - /** Fluently builds instances of {@link Extensions}. */ - public static final class Builder { - private Extensions base; - private IdentityHashMap, Object> newdata; - - private Builder(Extensions base) { - assert base != null; - this.base = base; - } - - private IdentityHashMap, Object> data(int size) { - if (newdata == null) { - newdata = new IdentityHashMap<>(size); - } - return newdata; - } - - /** - * Associates 'value' with 'key', replacing any previously associated value. - * - * @return this - */ - public Builder set(Key key, T value) { - data(1).put(key, value); - return this; - } - - /** Copies all entries from 'other' into this Builder. */ - public Builder setAll(Extensions other) { - data(other.data.size()).putAll(other.data); - return this; - } - - /** Builds the extensions. */ - public Extensions build() { - if (newdata != null) { - for (Map.Entry, Object> entry : base.data.entrySet()) { - if (!newdata.containsKey(entry.getKey())) { - newdata.put(entry.getKey(), entry.getValue()); - } - } - base = new Extensions(newdata); - newdata = null; - } - return base; - } - } - } } /** @@ -1048,4 +944,10 @@ public String toString() { } } } + + @Nullable + private static IdentityHashMap, Object> cloneCustomArgs( + @Nullable IdentityHashMap, Object> customArgs) { + return customArgs != null ? new IdentityHashMap<>(customArgs) : null; + } } diff --git a/api/src/test/java/io/grpc/NameResolverArgsExtensionsTest.java b/api/src/test/java/io/grpc/NameResolverArgsExtensionsTest.java deleted file mode 100644 index 56001c93c34..00000000000 --- a/api/src/test/java/io/grpc/NameResolverArgsExtensionsTest.java +++ /dev/null @@ -1,87 +0,0 @@ -/* - * Copyright 2024 The gRPC Authors - * - * Licensed under the Apache License, Version 2.0 (the "License"); - * you may not use this file except in compliance with the License. - * You may obtain a copy of the License at - * - * http://www.apache.org/licenses/LICENSE-2.0 - * - * Unless required by applicable law or agreed to in writing, software - * distributed under the License is distributed on an "AS IS" BASIS, - * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. - * See the License for the specific language governing permissions and - * limitations under the License. - */ - -package io.grpc; - -import static com.google.common.truth.Truth.assertThat; -import static org.junit.Assert.assertSame; - -import io.grpc.NameResolver.Args; -import io.grpc.NameResolver.Args.Extensions; -import org.junit.Test; -import org.junit.runner.RunWith; -import org.junit.runners.JUnit4; - -/** Unit tests for {@link NameResolver.Args.Extensions}. */ -@RunWith(JUnit4.class) -public class NameResolverArgsExtensionsTest { - private static final Args.Key FOO_KEY = Args.Key.create("foo"); - private static final Args.Key BAR_KEY = Args.Key.create("bar"); - private static final Args.Key QUX_KEY = Args.Key.create("qux"); - - @Test - public void buildExtensions() { - Extensions exts = Extensions.newBuilder().set(FOO_KEY, "To be, or not to be?").build(); - assertSame("To be, or not to be?", exts.get(FOO_KEY)); - assertThat(exts.keysForTest()).hasSize(1); - } - - @Test - public void duplicates() { - Extensions exts = - Extensions.newBuilder() - .set(FOO_KEY, "To be?") - .set(FOO_KEY, "Or not to be?") - .set(Args.Key.create("yolo"), "I'm not a duplicate") - .build(); - assertThat(exts.get(FOO_KEY)).isEqualTo("Or not to be?"); - assertThat(exts.keysForTest()).hasSize(2); - } - - @Test - public void toBuilder() { - Extensions exts = - Extensions.newBuilder().set(FOO_KEY, "To be?").build().toBuilder() - .set(FOO_KEY, "Or not to be?") - .set(Args.Key.create("yolo"), "I'm not a duplicate") - .build(); - assertThat(exts.get(FOO_KEY)).isEqualTo("Or not to be?"); - assertThat(exts.keysForTest()).hasSize(2); - } - - @Test - public void empty() { - assertThat(Extensions.EMPTY.keysForTest()).isEmpty(); - } - - @Test - public void setAll() { - Extensions newExts = - Extensions.newBuilder() - .set(FOO_KEY, "foo-orig") - .set(BAR_KEY, "bar-orig") - .setAll( - Extensions.newBuilder() - .set(FOO_KEY, "foo-updated") - .set(QUX_KEY, "qux-updated") - .build()) - .build(); - assertThat(newExts.get(FOO_KEY)).isEqualTo("foo-updated"); - assertThat(newExts.get(BAR_KEY)).isEqualTo("bar-orig"); - assertThat(newExts.get(QUX_KEY)).isEqualTo("qux-updated"); - assertThat(newExts.keysForTest()).hasSize(3); - } -} diff --git a/api/src/test/java/io/grpc/NameResolverTest.java b/api/src/test/java/io/grpc/NameResolverTest.java index d104b89de9f..083ba85fca5 100644 --- a/api/src/test/java/io/grpc/NameResolverTest.java +++ b/api/src/test/java/io/grpc/NameResolverTest.java @@ -50,8 +50,10 @@ public class NameResolverTest { private static final Attributes.Key YOLO_ATTR_KEY = Attributes.Key.create("yolo"); private static Attributes ATTRIBUTES = Attributes.newBuilder().set(YOLO_ATTR_KEY, "To be, or not to be?").build(); - private static final NameResolver.Args.Key EXT_ARG_KEY = + private static final NameResolver.Args.Key FOO_ARG_KEY = NameResolver.Args.Key.create("foo"); + private static final NameResolver.Args.Key BAR_ARG_KEY = + NameResolver.Args.Key.create("bar"); private static ConfigOrError CONFIG = ConfigOrError.fromConfig("foo"); @Rule @@ -82,7 +84,8 @@ public void args() { assertThat(args.getOffloadExecutor()).isSameInstanceAs(executor); assertThat(args.getOverrideAuthority()).isSameInstanceAs(overrideAuthority); assertThat(args.getMetricRecorder()).isSameInstanceAs(metricRecorder); - assertThat(args.getArg(EXT_ARG_KEY)).isEqualTo(extensionArgValue); + assertThat(args.getArg(FOO_ARG_KEY)).isEqualTo(extensionArgValue); + assertThat(args.getArg(BAR_ARG_KEY)).isNull(); NameResolver.Args args2 = args.toBuilder().build(); assertThat(args2.getDefaultPort()).isEqualTo(defaultPort); @@ -94,7 +97,8 @@ public void args() { assertThat(args2.getOffloadExecutor()).isSameInstanceAs(executor); assertThat(args2.getOverrideAuthority()).isSameInstanceAs(overrideAuthority); assertThat(args.getMetricRecorder()).isSameInstanceAs(metricRecorder); - assertThat(args.getArg(EXT_ARG_KEY)).isEqualTo(extensionArgValue); + assertThat(args.getArg(FOO_ARG_KEY)).isEqualTo(extensionArgValue); + assertThat(args.getArg(BAR_ARG_KEY)).isNull(); assertThat(args2).isNotSameInstanceAs(args); assertThat(args2).isNotEqualTo(args); @@ -111,7 +115,7 @@ private NameResolver.Args createArgs() { .setOffloadExecutor(executor) .setOverrideAuthority(overrideAuthority) .setMetricRecorder(metricRecorder) - .setArg(EXT_ARG_KEY, extensionArgValue) + .setArg(FOO_ARG_KEY, extensionArgValue) .build(); } diff --git a/core/src/main/java/io/grpc/internal/ManagedChannelImpl.java b/core/src/main/java/io/grpc/internal/ManagedChannelImpl.java index 403ed9a23c4..fb5b976369c 100644 --- a/core/src/main/java/io/grpc/internal/ManagedChannelImpl.java +++ b/core/src/main/java/io/grpc/internal/ManagedChannelImpl.java @@ -591,8 +591,7 @@ ClientStream newSubstream( this.authorityOverride = builder.authorityOverride; this.metricRecorder = new MetricRecorderImpl(builder.metricSinks, MetricInstrumentRegistry.getDefaultRegistry()); - this.nameResolverArgs = - NameResolver.Args.newBuilder() + NameResolver.Args.Builder nameResolverArgsBuilder = NameResolver.Args.newBuilder() .setDefaultPort(builder.getDefaultPort()) .setProxyDetector(proxyDetector) .setSynchronizationContext(syncContext) @@ -601,9 +600,9 @@ ClientStream newSubstream( .setChannelLogger(channelLogger) .setOffloadExecutor(this.offloadExecutorHolder) .setMetricRecorder(this.metricRecorder) - .setOverrideAuthority(this.authorityOverride) - .setAllExtensions(builder.nameResolverArgsExtBuilder.build()) - .build(); + .setOverrideAuthority(this.authorityOverride); + builder.copyAllNameResolverCustomArgsTo(nameResolverArgsBuilder); + this.nameResolverArgs = nameResolverArgsBuilder.build(); this.nameResolver = getNameResolver( targetUri, authorityOverride, nameResolverProvider, nameResolverArgs); this.balancerRpcExecutorPool = checkNotNull(balancerRpcExecutorPool, "balancerRpcExecutorPool"); diff --git a/core/src/main/java/io/grpc/internal/ManagedChannelImplBuilder.java b/core/src/main/java/io/grpc/internal/ManagedChannelImplBuilder.java index 7271c606854..d6ac2fde854 100644 --- a/core/src/main/java/io/grpc/internal/ManagedChannelImplBuilder.java +++ b/core/src/main/java/io/grpc/internal/ManagedChannelImplBuilder.java @@ -42,6 +42,7 @@ import io.grpc.MethodDescriptor; import io.grpc.MetricSink; import io.grpc.NameResolver; +import io.grpc.NameResolver.Args; import io.grpc.NameResolverProvider; import io.grpc.NameResolverRegistry; import io.grpc.ProxyDetector; @@ -55,6 +56,7 @@ import java.util.Arrays; import java.util.Collection; import java.util.Collections; +import java.util.IdentityHashMap; import java.util.LinkedHashMap; import java.util.List; import java.util.Map; @@ -151,8 +153,6 @@ public static ManagedChannelBuilder forTarget(String target) { private final List interceptors = new ArrayList<>(); NameResolverRegistry nameResolverRegistry = NameResolverRegistry.getDefaultRegistry(); - final NameResolver.Args.Extensions.Builder nameResolverArgsExtBuilder = - NameResolver.Args.Extensions.newBuilder(); final List transportFilters = new ArrayList<>(); @@ -161,6 +161,8 @@ public static ManagedChannelBuilder forTarget(String target) { final ChannelCredentials channelCredentials; @Nullable final CallCredentials callCredentials; + @Nullable + IdentityHashMap, Object> nameResolverCustomArgs; @Nullable private final SocketAddress directServerAddress; @@ -556,12 +558,6 @@ public ManagedChannelImplBuilder defaultServiceConfig(@Nullable Map s return this; } - @Override - public ManagedChannelImplBuilder setNameResolverArg(NameResolver.Args.Key key, X value) { - nameResolverArgsExtBuilder.set(key, value); - return this; - } - @Nullable private static Map checkMapEntryTypes(@Nullable Map map) { if (map == null) { @@ -621,6 +617,24 @@ private static List checkListEntryTypes(List list) { return Collections.unmodifiableList(parsedList); } + @Override + public ManagedChannelImplBuilder setNameResolverArg(NameResolver.Args.Key key, X value) { + if (nameResolverCustomArgs == null) { + nameResolverCustomArgs = new IdentityHashMap<>(); + } + nameResolverCustomArgs.put(key, value); + return this; + } + + @SuppressWarnings("unchecked") // This cast is safe because of setNameResolverArg()'s signature. + void copyAllNameResolverCustomArgsTo(NameResolver.Args.Builder dest) { + if (nameResolverCustomArgs != null) { + for (Map.Entry, Object> entry : nameResolverCustomArgs.entrySet()) { + dest.setArg((NameResolver.Args.Key) entry.getKey(), entry.getValue()); + } + } + } + @Override public ManagedChannelImplBuilder disableServiceConfigLookUp() { this.lookUpServiceConfig = false; diff --git a/core/src/test/java/io/grpc/internal/ManagedChannelImplBuilderTest.java b/core/src/test/java/io/grpc/internal/ManagedChannelImplBuilderTest.java index 9c486bff1a1..cf131a79d87 100644 --- a/core/src/test/java/io/grpc/internal/ManagedChannelImplBuilderTest.java +++ b/core/src/test/java/io/grpc/internal/ManagedChannelImplBuilderTest.java @@ -766,12 +766,12 @@ public void disableNameResolverServiceConfig() { @Test public void setNameResolverExtArgs() { - assertThat(builder.nameResolverArgsExtBuilder.build()) - .isSameInstanceAs(NameResolver.Args.Extensions.EMPTY); + assertThat(builder.nameResolverCustomArgs) + .isNull(); NameResolver.Args.Key testKey = NameResolver.Args.Key.create("test-key"); builder.setNameResolverArg(testKey, 42); - assertThat(builder.nameResolverArgsExtBuilder.build().get(testKey)).isEqualTo(42); + assertThat(builder.nameResolverCustomArgs.get(testKey)).isEqualTo(42); } @Test diff --git a/core/src/test/java/io/grpc/internal/ManagedChannelImplTest.java b/core/src/test/java/io/grpc/internal/ManagedChannelImplTest.java index e2ebadfe907..e9f5e1be91a 100644 --- a/core/src/test/java/io/grpc/internal/ManagedChannelImplTest.java +++ b/core/src/test/java/io/grpc/internal/ManagedChannelImplTest.java @@ -204,7 +204,7 @@ public class ManagedChannelImplTest { .setServiceConfigParser(mock(NameResolver.ServiceConfigParser.class)) .setScheduledExecutorService(new FakeClock().getScheduledExecutorService()) .build(); - private static final NameResolver.Args.Key TEST_RESOLVER_ARG_EXT_KEY = + private static final NameResolver.Args.Key TEST_RESOLVER_CUSTOM_ARG_KEY = NameResolver.Args.Key.create("test-key"); private URI expectedUri; @@ -4273,7 +4273,7 @@ public String getDefaultScheme() { channelBuilder .nameResolverFactory(factory) .proxyDetector(neverProxy) - .setNameResolverArg(TEST_RESOLVER_ARG_EXT_KEY, "test-value"); + .setNameResolverArg(TEST_RESOLVER_CUSTOM_ARG_KEY, "test-value"); createChannel(); @@ -4281,7 +4281,7 @@ public String getDefaultScheme() { assertThat(args).isNotNull(); assertThat(args.getDefaultPort()).isEqualTo(DEFAULT_PORT); assertThat(args.getProxyDetector()).isSameInstanceAs(neverProxy); - assertThat(args.getArg(TEST_RESOLVER_ARG_EXT_KEY)).isEqualTo("test-value"); + assertThat(args.getArg(TEST_RESOLVER_CUSTOM_ARG_KEY)).isEqualTo("test-value"); verify(offloadExecutor, never()).execute(any(Runnable.class)); args.getOffloadExecutor() From 7653270a9c9b25e4a9266a9b3681f3ea223658dc Mon Sep 17 00:00:00 2001 From: John Cormie Date: Tue, 17 Dec 2024 15:40:13 -0800 Subject: [PATCH 18/23] A few more extended -> custom renames --- .../java/io/grpc/ManagedChannelBuilder.java | 6 ++--- api/src/main/java/io/grpc/NameResolver.java | 24 +++++++++---------- 2 files changed, 15 insertions(+), 15 deletions(-) diff --git a/api/src/main/java/io/grpc/ManagedChannelBuilder.java b/api/src/main/java/io/grpc/ManagedChannelBuilder.java index fe2e8949a56..df867d09ba1 100644 --- a/api/src/main/java/io/grpc/ManagedChannelBuilder.java +++ b/api/src/main/java/io/grpc/ManagedChannelBuilder.java @@ -634,11 +634,11 @@ protected T addMetricSink(MetricSink metricSink) { } /** - * Provides an "extended" argument for the {@link NameResolver}, if applicable, replacing any - * 'value' previously provided for 'key'. + * Provides a "custom" argument for the {@link NameResolver}, if applicable, replacing any 'value' + * previously provided for 'key'. * *

NB: If the selected {@link NameResolver} does not understand 'key', or target URI resolution - * isn't needed at all, your extended argument will be silently ignored. + * isn't needed at all, your custom argument will be silently ignored. * *

See {@link NameResolver.Args#getArg(NameResolver.Args.Key)} for more. * diff --git a/api/src/main/java/io/grpc/NameResolver.java b/api/src/main/java/io/grpc/NameResolver.java index 625495f4347..95329708234 100644 --- a/api/src/main/java/io/grpc/NameResolver.java +++ b/api/src/main/java/io/grpc/NameResolver.java @@ -279,7 +279,7 @@ public Status onResult2(ResolutionResult resolutionResult) { * Information that a {@link Factory} uses to create a {@link NameResolver}. * *

Args applicable to all {@link NameResolver}s are defined here using ordinary setters and - * getters. This container can also hold externally-defined "extended" args that aren't so widely + * getters. This container can also hold externally-defined "custom" args that aren't so widely * useful or that would be inappropriate dependencies for this low level API. See {@link * Args#getArg} for more. * @@ -320,7 +320,7 @@ private Args(Builder builder) { * * @since 1.21.0 */ - //

TODO: Only meaningful for InetSocketAddress producers. Move to {@link Extensions}? + //

TODO: Only meaningful for InetSocketAddress producers. Make this a custom arg? public int getDefaultPort() { return defaultPort; } @@ -374,24 +374,24 @@ public ServiceConfigParser getServiceConfigParser() { } /** - * Gets the value of an "extension" arg by key, or {@code null} if it's not set. + * Gets the value of a custom arg by key, or {@code null} if it's not set. * - *

While ordinary {@link Args} should be universally useful and meaningful, extension - * arguments can apply just to resolvers of a certain URI scheme, just to resolvers producing a - * particular type of {@link java.net.SocketAddress}, or even an individual {@link NameResolver} - * subclass. Extension args are identified by an instance of {@link Args.Key} which should be - * defined in a java package and class appropriate to the argument's scope. + *

While ordinary {@link Args} should be universally useful and meaningful, custom arguments + * can apply just to resolvers of a certain URI scheme, just to resolvers producing a particular + * type of {@link java.net.SocketAddress}, or even an individual {@link NameResolver} subclass. + * Custom args are identified by an instance of {@link Args.Key} which should be a constant + * defined in a java package and class appropriate for the argument's scope. * *

{@link Args} are normally reserved for information in *support* of name resolution, not * the name to be resolved itself. However, there are rare cases where all or part of the target * name can't be represented by any standard URI scheme or can't be encoded as a String at all. - * Extension args, in contrast, can be an arbitrary Java type, making them a useful work around - * in these cases. + * Custom args, in contrast, can be an arbitrary Java type, making them a useful work around in + * these cases. * - *

Extension args can also be used simply to avoid adding inappropriate deps to the low level + *

Custom args can also be used simply to avoid adding inappropriate deps to the low level * io.grpc package. */ - @SuppressWarnings("unchecked") // Cast is safe because all put()s go through the setArg() API. + @SuppressWarnings("unchecked") // Cast is safe because all put()s go through the setArg() API. @Nullable public T getArg(Key key) { return customArgs != null ? (T) customArgs.get(key) : null; From 5d4a24d928f8670370526a49a2e5f1f5276dfc93 Mon Sep 17 00:00:00 2001 From: John Cormie Date: Tue, 17 Dec 2024 15:48:22 -0800 Subject: [PATCH 19/23] one more extension rename --- api/src/main/java/io/grpc/NameResolver.java | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/api/src/main/java/io/grpc/NameResolver.java b/api/src/main/java/io/grpc/NameResolver.java index 95329708234..2a6e4ebebac 100644 --- a/api/src/main/java/io/grpc/NameResolver.java +++ b/api/src/main/java/io/grpc/NameResolver.java @@ -385,7 +385,7 @@ public ServiceConfigParser getServiceConfigParser() { *

{@link Args} are normally reserved for information in *support* of name resolution, not * the name to be resolved itself. However, there are rare cases where all or part of the target * name can't be represented by any standard URI scheme or can't be encoded as a String at all. - * Custom args, in contrast, can be an arbitrary Java type, making them a useful work around in + * Custom args, in contrast, can hold arbitrary Java types, making them a useful work around in * these cases. * *

Custom args can also be used simply to avoid adding inappropriate deps to the low level @@ -618,7 +618,7 @@ public Args build() { } /** - * Identifies an externally-defined extension argument that can be stored in {@link Args}. + * Identifies an externally-defined custom argument that can be stored in {@link Args}. * *

Uses reference equality so keys should be defined as global constants. * From 38946a4f471c577333e6d4fc11c64ba3e034d63e Mon Sep 17 00:00:00 2001 From: John Cormie Date: Tue, 17 Dec 2024 15:56:43 -0800 Subject: [PATCH 20/23] Disallow null arg keys/values just like CallOptions does. setter order --- api/src/main/java/io/grpc/NameResolver.java | 2 ++ core/src/main/java/io/grpc/internal/ManagedChannelImpl.java | 4 ++-- .../main/java/io/grpc/internal/ManagedChannelImplBuilder.java | 3 +-- 3 files changed, 5 insertions(+), 4 deletions(-) diff --git a/api/src/main/java/io/grpc/NameResolver.java b/api/src/main/java/io/grpc/NameResolver.java index 2a6e4ebebac..7eb4860dd3e 100644 --- a/api/src/main/java/io/grpc/NameResolver.java +++ b/api/src/main/java/io/grpc/NameResolver.java @@ -592,6 +592,8 @@ public Builder setOverrideAuthority(String authority) { /** See {@link Args#getArg(Key)}. */ public Builder setArg(Key key, T value) { + checkNotNull(key, "key"); + checkNotNull(value, "value"); if (customArgs == null) { customArgs = new IdentityHashMap<>(); } diff --git a/core/src/main/java/io/grpc/internal/ManagedChannelImpl.java b/core/src/main/java/io/grpc/internal/ManagedChannelImpl.java index fb5b976369c..5c2871a1373 100644 --- a/core/src/main/java/io/grpc/internal/ManagedChannelImpl.java +++ b/core/src/main/java/io/grpc/internal/ManagedChannelImpl.java @@ -599,8 +599,8 @@ ClientStream newSubstream( .setServiceConfigParser(serviceConfigParser) .setChannelLogger(channelLogger) .setOffloadExecutor(this.offloadExecutorHolder) - .setMetricRecorder(this.metricRecorder) - .setOverrideAuthority(this.authorityOverride); + .setOverrideAuthority(this.authorityOverride) + .setMetricRecorder(this.metricRecorder); builder.copyAllNameResolverCustomArgsTo(nameResolverArgsBuilder); this.nameResolverArgs = nameResolverArgsBuilder.build(); this.nameResolver = getNameResolver( diff --git a/core/src/main/java/io/grpc/internal/ManagedChannelImplBuilder.java b/core/src/main/java/io/grpc/internal/ManagedChannelImplBuilder.java index d6ac2fde854..002a370cd6c 100644 --- a/core/src/main/java/io/grpc/internal/ManagedChannelImplBuilder.java +++ b/core/src/main/java/io/grpc/internal/ManagedChannelImplBuilder.java @@ -42,7 +42,6 @@ import io.grpc.MethodDescriptor; import io.grpc.MetricSink; import io.grpc.NameResolver; -import io.grpc.NameResolver.Args; import io.grpc.NameResolverProvider; import io.grpc.NameResolverRegistry; import io.grpc.ProxyDetector; @@ -629,7 +628,7 @@ public ManagedChannelImplBuilder setNameResolverArg(NameResolver.Args.Key @SuppressWarnings("unchecked") // This cast is safe because of setNameResolverArg()'s signature. void copyAllNameResolverCustomArgsTo(NameResolver.Args.Builder dest) { if (nameResolverCustomArgs != null) { - for (Map.Entry, Object> entry : nameResolverCustomArgs.entrySet()) { + for (Map.Entry, Object> entry : nameResolverCustomArgs.entrySet()) { dest.setArg((NameResolver.Args.Key) entry.getKey(), entry.getValue()); } } From 2575fb188f4d8193664b9b480df29d5fc66ce3bf Mon Sep 17 00:00:00 2001 From: John Cormie Date: Tue, 17 Dec 2024 15:59:07 -0800 Subject: [PATCH 21/23] returns --- api/src/main/java/io/grpc/NameResolver.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/api/src/main/java/io/grpc/NameResolver.java b/api/src/main/java/io/grpc/NameResolver.java index 7eb4860dd3e..e8295365ca8 100644 --- a/api/src/main/java/io/grpc/NameResolver.java +++ b/api/src/main/java/io/grpc/NameResolver.java @@ -374,7 +374,7 @@ public ServiceConfigParser getServiceConfigParser() { } /** - * Gets the value of a custom arg by key, or {@code null} if it's not set. + * Returns the value of a custom arg named 'key', or {@code null} if it's not set. * *

While ordinary {@link Args} should be universally useful and meaningful, custom arguments * can apply just to resolvers of a certain URI scheme, just to resolvers producing a particular From e7fbb5c37e73c6d3f15e6eb3c8f4fde1d9b9a688 Mon Sep 17 00:00:00 2001 From: John Cormie Date: Tue, 17 Dec 2024 16:01:32 -0800 Subject: [PATCH 22/23] extension --- api/src/test/java/io/grpc/NameResolverTest.java | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/api/src/test/java/io/grpc/NameResolverTest.java b/api/src/test/java/io/grpc/NameResolverTest.java index 083ba85fca5..ae8c080bd5c 100644 --- a/api/src/test/java/io/grpc/NameResolverTest.java +++ b/api/src/test/java/io/grpc/NameResolverTest.java @@ -69,7 +69,7 @@ public class NameResolverTest { private final Executor executor = Executors.newSingleThreadExecutor(); private final String overrideAuthority = "grpc.io"; private final MetricRecorder metricRecorder = new MetricRecorder() {}; - private final int extensionArgValue = 42; + private final int customArgValue = 42; @Mock NameResolver.Listener mockListener; @Test @@ -84,7 +84,7 @@ public void args() { assertThat(args.getOffloadExecutor()).isSameInstanceAs(executor); assertThat(args.getOverrideAuthority()).isSameInstanceAs(overrideAuthority); assertThat(args.getMetricRecorder()).isSameInstanceAs(metricRecorder); - assertThat(args.getArg(FOO_ARG_KEY)).isEqualTo(extensionArgValue); + assertThat(args.getArg(FOO_ARG_KEY)).isEqualTo(customArgValue); assertThat(args.getArg(BAR_ARG_KEY)).isNull(); NameResolver.Args args2 = args.toBuilder().build(); @@ -97,7 +97,7 @@ public void args() { assertThat(args2.getOffloadExecutor()).isSameInstanceAs(executor); assertThat(args2.getOverrideAuthority()).isSameInstanceAs(overrideAuthority); assertThat(args.getMetricRecorder()).isSameInstanceAs(metricRecorder); - assertThat(args.getArg(FOO_ARG_KEY)).isEqualTo(extensionArgValue); + assertThat(args.getArg(FOO_ARG_KEY)).isEqualTo(customArgValue); assertThat(args.getArg(BAR_ARG_KEY)).isNull(); assertThat(args2).isNotSameInstanceAs(args); @@ -115,7 +115,7 @@ private NameResolver.Args createArgs() { .setOffloadExecutor(executor) .setOverrideAuthority(overrideAuthority) .setMetricRecorder(metricRecorder) - .setArg(FOO_ARG_KEY, extensionArgValue) + .setArg(FOO_ARG_KEY, customArgValue) .build(); } From 0857a68bd1b575dd010c4b2d46fcd31eb17f30c9 Mon Sep 17 00:00:00 2001 From: John Cormie Date: Thu, 19 Dec 2024 21:27:40 -0800 Subject: [PATCH 23/23] checkNotNull --- .../main/java/io/grpc/internal/ManagedChannelImplBuilder.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/core/src/main/java/io/grpc/internal/ManagedChannelImplBuilder.java b/core/src/main/java/io/grpc/internal/ManagedChannelImplBuilder.java index 002a370cd6c..20f7e901ddd 100644 --- a/core/src/main/java/io/grpc/internal/ManagedChannelImplBuilder.java +++ b/core/src/main/java/io/grpc/internal/ManagedChannelImplBuilder.java @@ -621,7 +621,7 @@ public ManagedChannelImplBuilder setNameResolverArg(NameResolver.Args.Key if (nameResolverCustomArgs == null) { nameResolverCustomArgs = new IdentityHashMap<>(); } - nameResolverCustomArgs.put(key, value); + nameResolverCustomArgs.put(checkNotNull(key, "key"), checkNotNull(value, "value")); return this; }