core: partially stabilize Attributes API (#4458)

Deprecate static builder method, Keys.of(), add a notice of plans to
remove keys(), emphasize that the name is only a debug label.
The `@ExperimentalAPI` is left on the class because there are still
issues around hashCode/equals.

alts/src/main/java/io/grpc/alts/internal/AltsProtocolNegotiator.java

@@ -39,9 +39,9 @@ import io.netty.util.AsciiString;
  */
 public abstract class AltsProtocolNegotiator implements ProtocolNegotiator {
 
-  private static final Attributes.Key<TsiPeer> TSI_PEER_KEY = Attributes.Key.of("TSI_PEER");
+  private static final Attributes.Key<TsiPeer> TSI_PEER_KEY = Attributes.Key.create("TSI_PEER");
   private static final Attributes.Key<AltsAuthContext> ALTS_CONTEXT_KEY =
-      Attributes.Key.of("ALTS_CONTEXT_KEY");
+      Attributes.Key.create("ALTS_CONTEXT_KEY");
   private static final AsciiString scheme = AsciiString.of("https");
 
   public static Attributes.Key<TsiPeer> getTsiPeerAttributeKey() {

core/src/jmh/java/io/grpc/AttributesBenchmark.java

@@ -46,8 +46,8 @@ public class AttributesBenchmark {
   public void setUp() {
     keys = new Attributes.Key[iterations];
     for (int i = 0; i < iterations; i++) {
-      keys[i] = Attributes.Key.of("any");
-      withValue = Attributes.newBuilder(withValue).set(keys[i], "yes").build();
+      keys[i] = Attributes.Key.create("any");
+      withValue = withValue.toBuilder().set(keys[i], "yes").build();
     }
   }
 
@@ -63,7 +63,7 @@ public class AttributesBenchmark {
   public Attributes chain() {
     Attributes attr = base;
     for (int i = 0; i < iterations; i++) {
-      attr = Attributes.newBuilder(attr).set(keys[i], new Object()).build();
+      attr = attr.toBuilder().set(keys[i], new Object()).build();
     }
     return attr;
   }

core/src/main/java/io/grpc/Attributes.java

@@ -29,6 +29,7 @@ import javax.annotation.concurrent.Immutable;
 
 /**
  * An immutable type-safe container of attributes.
+ * @since 1.13.0
  */
 @ExperimentalApi("https://github.com/grpc/grpc-java/issues/1764")
 @Immutable
@@ -56,14 +57,25 @@ public final class Attributes {
    * Returns set of keys stored in container.
    *
    * @return Set of Key objects.
+   * @deprecated This method is being considered for removal, if you feel this method is needed
+   *     please reach out on this Github issue:
+   *     <a href="https://github.com/grpc/grpc-java/issues/1764">grpc-java/issues/1764</a>.
    */
+  @Deprecated
   public Set<Key<?>> keys() {
     return Collections.unmodifiableSet(data.keySet());
   }
 
+  Set<Key<?>> keysForTest() {
+    return Collections.unmodifiableSet(data.keySet());
+  }
+
   /**
    * Create a new builder that is pre-populated with the content from a given container.
+   * @deprecated Use {@link Attributes#toBuilder()} on the {@link Attributes} instance instead.
+   *     This method will be removed in the future.
    */
+  @Deprecated
   public static Builder newBuilder(Attributes base) {
     checkNotNull(base, "base");
     return new Builder(base);
@@ -76,32 +88,53 @@ public final class Attributes {
     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);
+  }
+
   /**
    * Key for an key-value pair.
    * @param <T> type of the value in the key-value pair
    */
   @Immutable
   public static final class Key<T> {
-    private final String name;
+    private final String debugString;
 
-    private Key(String name) {
-      this.name = name;
+    private Key(String debugString) {
+      this.debugString = debugString;
     }
 
     @Override
     public String toString() {
-      return name;
+      return debugString;
     }
 
     /**
      * Factory method for creating instances of {@link Key}.
      *
-     * @param name the name of Key. Name collision, won't cause key collision.
+     * @param debugString a string used to describe the key, used for debugging.
+     * @param <T> Key type
+     * @return Key object
+     * @deprecated use {@link #create} instead. This method will be removed in the future.
+     */
+    @Deprecated
+    public static <T> Key<T> of(String debugString) {
+      return new Key<T>(debugString);
+    }
+
+    /**
+     * Factory method for creating instances of {@link Key}.
+     *
+     * @param debugString a string used to describe the key, used for debugging.
      * @param <T> Key type
      * @return Key object
      */
-    public static <T> Key<T> of(String name) {
-      return new Key<T>(name);
+    public static <T> Key<T> create(String debugString) {
+      return new Key<T>(debugString);
     }
   }
 
@@ -117,6 +150,8 @@ public final class Attributes {
    * equal at one point in time and not equal at another (due to concurrent mutation of attribute
    * values).
    *
+   * <p>This method is not implemented efficiently and is meant for testing.
+   *
    * @param o an object.
    * @return true if the given object is a {@link Attributes} equal attributes.
    */

core/src/main/java/io/grpc/CallCredentials.java

@@ -43,7 +43,7 @@ public interface CallCredentials {
    */
   @ExperimentalApi("https://github.com/grpc/grpc-java/issues/1914")
   public static final Key<SecurityLevel> ATTR_SECURITY_LEVEL =
-      Key.of("io.grpc.CallCredentials.securityLevel");
+      Key.create("io.grpc.CallCredentials.securityLevel");
 
   /**
    * The authority string used to authenticate the server. Usually it's the server's host name. It
@@ -52,7 +52,7 @@ public interface CallCredentials {
    * io.grpc.CallOptions} with increasing precedence.
    */
   @ExperimentalApi("https://github.com/grpc/grpc-java/issues/1914")
-  public static final Key<String> ATTR_AUTHORITY = Key.of("io.grpc.CallCredentials.authority");
+  public static final Key<String> ATTR_AUTHORITY = Key.create("io.grpc.CallCredentials.authority");
 
   /**
    * Pass the credential data to the given {@link MetadataApplier}, which will propagate it to

core/src/main/java/io/grpc/Grpc.java

@@ -32,12 +32,12 @@ public final class Grpc {
    */
   @ExperimentalApi("https://github.com/grpc/grpc-java/issues/1710")
   public static final Attributes.Key<SocketAddress> TRANSPORT_ATTR_REMOTE_ADDR =
-          Attributes.Key.of("remote-addr");
+          Attributes.Key.create("remote-addr");
 
   /**
    * Attribute key for SSL session of a transport.
    */
   @ExperimentalApi("https://github.com/grpc/grpc-java/issues/1710")
   public static final Attributes.Key<SSLSession> TRANSPORT_ATTR_SSL_SESSION =
-          Attributes.Key.of("ssl-session");
+          Attributes.Key.create("ssl-session");
 }

core/src/main/java/io/grpc/NameResolver.java

@@ -94,7 +94,7 @@ public abstract class NameResolver {
      * @since 1.0.0
      */
     public static final Attributes.Key<Integer> PARAMS_DEFAULT_PORT =
-        Attributes.Key.of("params-default-port");
+        Attributes.Key.create("params-default-port");
 
     /**
      * Creates a {@link NameResolver} for the given target URI, or {@code null} if the given URI

core/src/main/java/io/grpc/internal/AutoConfiguredLoadBalancerFactory.java

@@ -17,6 +17,7 @@
 package io.grpc.internal;
 
 import com.google.common.annotations.VisibleForTesting;
+import com.google.common.base.Preconditions;
 import io.grpc.Attributes;
 import io.grpc.ConnectivityState;
 import io.grpc.ConnectivityStateInfo;
@@ -74,9 +75,9 @@ final class AutoConfiguredLoadBalancerFactory extends LoadBalancer.Factory {
     @Override
     public void handleResolvedAddressGroups(
         List<EquivalentAddressGroup> servers, Attributes attributes) {
-      if (attributes.keys().contains(GrpcAttributes.NAME_RESOLVER_SERVICE_CONFIG)) {
-        Factory newlbf = decideLoadBalancerFactory(
-            servers, attributes.get(GrpcAttributes.NAME_RESOLVER_SERVICE_CONFIG));
+      Map<String, Object> configMap = attributes.get(GrpcAttributes.NAME_RESOLVER_SERVICE_CONFIG);
+      if (configMap != null) {
+        Factory newlbf = decideLoadBalancerFactory(servers, configMap);
         if (newlbf != null && newlbf != delegateFactory) {
           helper.updateBalancingState(ConnectivityState.CONNECTING, new EmptySubchannelPicker());
           getDelegate().shutdown();
@@ -141,7 +142,7 @@ final class AutoConfiguredLoadBalancerFactory extends LoadBalancer.Factory {
     @VisibleForTesting
     static LoadBalancer.Factory decideLoadBalancerFactory(
         List<EquivalentAddressGroup> servers, Map<String, Object> config) {
-
+      Preconditions.checkNotNull(config);
       // Check for balancer addresses
       boolean haveBalancerAddress = false;
       for (EquivalentAddressGroup s : servers) {

core/src/main/java/io/grpc/internal/GrpcAttributes.java

@@ -27,14 +27,14 @@ public final class GrpcAttributes {
    * Attribute key for service config.
    */
   public static final Attributes.Key<Map<String, Object>> NAME_RESOLVER_SERVICE_CONFIG =
-      Attributes.Key.of("service-config");
+      Attributes.Key.create("service-config");
 
   /**
    * The naming authority of a gRPC LB server address.  It is an address-group-level attribute,
    * present when the address group is a LoadBalancer.
    */
   public static final Attributes.Key<String> ATTR_LB_ADDR_AUTHORITY =
-      Attributes.Key.of("io.grpc.grpclb.lbAddrAuthority");
+      Attributes.Key.create("io.grpc.grpclb.lbAddrAuthority");
 
 
   private GrpcAttributes() {}

core/src/main/java/io/grpc/internal/ProxyDetector.java

@@ -28,7 +28,7 @@ import javax.annotation.Nullable;
  * {@link io.grpc.NameResolver}.
  */
 public interface ProxyDetector {
-  Attributes.Key<ProxyParameters> PROXY_PARAMS_KEY = Attributes.Key.of("proxy-params-key");
+  Attributes.Key<ProxyParameters> PROXY_PARAMS_KEY = Attributes.Key.create("proxy-params-key");
   /**
    * Given a target address, returns which proxy address should be used. If no proxy should be
    * used, then return value will be null. The address of the {@link ProxyParameters} is always

core/src/main/java/io/grpc/util/RoundRobinLoadBalancerFactory.java

@@ -98,7 +98,7 @@ public final class RoundRobinLoadBalancerFactory extends LoadBalancer.Factory {
   static final class RoundRobinLoadBalancer extends LoadBalancer {
     @VisibleForTesting
     static final Attributes.Key<Ref<ConnectivityStateInfo>> STATE_INFO =
-        Attributes.Key.of("state-info");
+        Attributes.Key.create("state-info");
 
     private static final Logger logger = Logger.getLogger(RoundRobinLoadBalancer.class.getName());
 

core/src/test/java/io/grpc/AttributesTest.java

@@ -27,13 +27,13 @@ import org.junit.runners.JUnit4;
 /** Unit tests for {@link Attributes}. */
 @RunWith(JUnit4.class)
 public class AttributesTest {
-  private static final Attributes.Key<String> YOLO_KEY = Attributes.Key.of("yolo");
+  private static final Attributes.Key<String> YOLO_KEY = Attributes.Key.create("yolo");
 
   @Test
   public void buildAttributes() {
     Attributes attrs = Attributes.newBuilder().set(YOLO_KEY, "To be, or not to be?").build();
     assertSame("To be, or not to be?", attrs.get(YOLO_KEY));
-    assertEquals(1, attrs.keys().size());
+    assertEquals(1, attrs.keysForTest().size());
   }
 
   @Test
@@ -41,15 +41,28 @@ public class AttributesTest {
     Attributes attrs = Attributes.newBuilder()
         .set(YOLO_KEY, "To be?")
         .set(YOLO_KEY, "Or not to be?")
-        .set(Attributes.Key.of("yolo"), "I'm not a duplicate")
+        .set(Attributes.Key.create("yolo"), "I'm not a duplicate")
         .build();
     assertSame("Or not to be?", attrs.get(YOLO_KEY));
-    assertEquals(2, attrs.keys().size());
+    assertEquals(2, attrs.keysForTest().size());
+  }
+
+  @Test
+  public void toBuilder() {
+    Attributes attrs = Attributes.newBuilder()
+        .set(YOLO_KEY, "To be?")
+        .build()
+        .toBuilder()
+        .set(YOLO_KEY, "Or not to be?")
+        .set(Attributes.Key.create("yolo"), "I'm not a duplicate")
+        .build();
+    assertSame("Or not to be?", attrs.get(YOLO_KEY));
+    assertEquals(2, attrs.keysForTest().size());
   }
 
   @Test
   public void empty() {
-    assertEquals(0, Attributes.EMPTY.keys().size());
+    assertEquals(0, Attributes.EMPTY.keysForTest().size());
   }
 
   @Test
@@ -64,7 +77,7 @@ public class AttributesTest {
       }
     }
 
-    Attributes.Key<EqualObject> key = Attributes.Key.of("ints");
+    Attributes.Key<EqualObject> key = Attributes.Key.create("ints");
     EqualObject v1 = new EqualObject();
     EqualObject v2 = new EqualObject();
 

core/src/test/java/io/grpc/PickFirstLoadBalancerTest.java

@@ -60,7 +60,7 @@ public class PickFirstLoadBalancerTest {
   private List<EquivalentAddressGroup> servers = Lists.newArrayList();
   private List<SocketAddress> socketAddresses = Lists.newArrayList();
 
-  private static final Attributes.Key<String> FOO = Attributes.Key.of("foo");
+  private static final Attributes.Key<String> FOO = Attributes.Key.create("foo");
   private Attributes affinity = Attributes.newBuilder().set(FOO, "bar").build();
 
   @Captor

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

@@ -77,7 +77,7 @@ public class CallCredentialsApplyingTest {
   private static final String AUTHORITY = "testauthority";
   private static final String USER_AGENT = "testuseragent";
   private static final ProxyParameters NO_PROXY = null;
-  private static final Attributes.Key<String> ATTR_KEY = Attributes.Key.of("somekey");
+  private static final Attributes.Key<String> ATTR_KEY = Attributes.Key.create("somekey");
   private static final String ATTR_VALUE = "somevalue";
   private static final MethodDescriptor<String, Integer> method =
       MethodDescriptor.<String, Integer>newBuilder()

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

@@ -931,7 +931,7 @@ public class ClientCallImplTest {
         method, MoreExecutors.directExecutor(), baseCallOptions, provider,
         deadlineCancellationExecutor, channelCallTracer, false /* retryEnabled */);
     Attributes attrs =
-        Attributes.newBuilder().set(Key.<String>of("fake key"), "fake value").build();
+        Attributes.newBuilder().set(Key.<String>create("fake key"), "fake value").build();
     when(stream.getAttributes()).thenReturn(attrs);
 
     assertNotEquals(attrs, call.getAttributes());

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

@@ -182,7 +182,7 @@ public class DelayedStreamTest {
   @Test
   public void setStream_getAttributes() {
     Attributes attributes =
-        Attributes.newBuilder().set(Key.<String>of("fakeKey"), "fakeValue").build();
+        Attributes.newBuilder().set(Key.<String>create("fakeKey"), "fakeValue").build();
     when(realStream.getAttributes()).thenReturn(attributes);
 
     stream.start(listener);

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

@@ -126,7 +126,7 @@ public class ManagedChannelImplTest {
           .setResponseMarshaller(new IntegerMarshaller())
           .build();
   private static final Attributes.Key<String> SUBCHANNEL_ATTR_KEY =
-      Attributes.Key.of("subchannel-attr-key");
+      Attributes.Key.create("subchannel-attr-key");
   private static final long RECONNECT_BACKOFF_INTERVAL_NANOS = 10;
   private static final String SERVICE_NAME = "fake.example.com";
   private static final String AUTHORITY = SERVICE_NAME;

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

@@ -562,9 +562,9 @@ public class ServerImplTest {
   @Test
   public void transportFilters() throws Exception {
     final SocketAddress remoteAddr = mock(SocketAddress.class);
-    final Attributes.Key<String> key1 = Attributes.Key.of("test-key1");
-    final Attributes.Key<String> key2 = Attributes.Key.of("test-key2");
-    final Attributes.Key<String> key3 = Attributes.Key.of("test-key3");
+    final Attributes.Key<String> key1 = Attributes.Key.create("test-key1");
+    final Attributes.Key<String> key2 = Attributes.Key.create("test-key2");
+    final Attributes.Key<String> key3 = Attributes.Key.create("test-key3");
     final AtomicReference<Attributes> filter1TerminationCallbackArgument =
         new AtomicReference<Attributes>();
     final AtomicReference<Attributes> filter2TerminationCallbackArgument =
@@ -578,7 +578,7 @@ public class ServerImplTest {
               .set(Grpc.TRANSPORT_ATTR_REMOTE_ADDR, remoteAddr)
               .build(), attrs);
           readyCallbackCalled.incrementAndGet();
-          return Attributes.newBuilder(attrs)
+          return attrs.toBuilder()
               .set(key1, "yalayala")
               .set(key2, "blabla")
               .build();
@@ -599,7 +599,7 @@ public class ServerImplTest {
               .set(key2, "blabla")
               .build(), attrs);
           readyCallbackCalled.incrementAndGet();
-          return Attributes.newBuilder(attrs)
+          return attrs.toBuilder()
               .set(key1, "ouch")
               .set(key3, "puff")
               .build();

core/src/test/java/io/grpc/util/RoundRobinLoadBalancerTest.java

@@ -83,7 +83,7 @@ public class RoundRobinLoadBalancerTest {
   private RoundRobinLoadBalancer loadBalancer;
   private List<EquivalentAddressGroup> servers = Lists.newArrayList();
   private Map<EquivalentAddressGroup, Subchannel> subchannels = Maps.newLinkedHashMap();
-  private static final Attributes.Key<String> MAJOR_KEY = Attributes.Key.of("major-key");
+  private static final Attributes.Key<String> MAJOR_KEY = Attributes.Key.create("major-key");
   private Attributes affinity = Attributes.newBuilder().set(MAJOR_KEY, "I got the keys").build();
 
   @Captor

grpclb/src/main/java/io/grpc/grpclb/GrpclbConstants.java

@@ -38,7 +38,7 @@ public final class GrpclbConstants {
    * An attribute of a name resolution result, designating the LB policy.
    */
   public static final Attributes.Key<LbPolicy> ATTR_LB_POLICY =
-      Attributes.Key.of("io.grpc.grpclb.lbPolicy");
+      Attributes.Key.create("io.grpc.grpclb.lbPolicy");
 
   /**
    * The opaque token given by the remote balancer for each returned server address.  The client

grpclb/src/main/java/io/grpc/grpclb/GrpclbState.java

@@ -100,7 +100,7 @@ final class GrpclbState {
   private final ScheduledExecutorService timerService;
 
   private static final Attributes.Key<AtomicReference<ConnectivityStateInfo>> STATE_INFO =
-      Attributes.Key.of("io.grpc.grpclb.GrpclbLoadBalancer.stateInfo");
+      Attributes.Key.create("io.grpc.grpclb.GrpclbLoadBalancer.stateInfo");
 
   // Scheduled only once.  Never reset.
   @Nullable

grpclb/src/test/java/io/grpc/grpclb/CachedSubchannelPoolTest.java

@@ -54,7 +54,7 @@ public class CachedSubchannelPoolTest {
       new EquivalentAddressGroup(new FakeSocketAddress("fake-address-1"), Attributes.EMPTY);
   private static final EquivalentAddressGroup EAG2 =
       new EquivalentAddressGroup(new FakeSocketAddress("fake-address-2"), Attributes.EMPTY);
-  private static final Attributes.Key<String> ATTR_KEY = Attributes.Key.of("test-attr");
+  private static final Attributes.Key<String> ATTR_KEY = Attributes.Key.create("test-attr");
   private static final Attributes ATTRS1 = Attributes.newBuilder().set(ATTR_KEY, "1").build();
   private static final Attributes ATTRS2 = Attributes.newBuilder().set(ATTR_KEY, "2").build();
   private static final FakeClock.TaskFilter SHUTDOWN_SCHEDULED_TASK_FILTER =

grpclb/src/test/java/io/grpc/grpclb/GrpclbLoadBalancerTest.java

@@ -102,7 +102,7 @@ import org.mockito.stubbing.Answer;
 @RunWith(JUnit4.class)
 public class GrpclbLoadBalancerTest {
   private static final Attributes.Key<String> RESOLUTION_ATTR =
-      Attributes.Key.of("resolution-attr");
+      Attributes.Key.create("resolution-attr");
   private static final String SERVICE_AUTHORITY = "api.google.com";
   private static final FakeClock.TaskFilter LOAD_REPORTING_TASK_FILTER =
       new FakeClock.TaskFilter() {

testing/src/main/java/io/grpc/internal/testing/AbstractTransportTest.java

@@ -95,7 +95,7 @@ public abstract class AbstractTransportTest {
   private static final int TIMEOUT_MS = 1000;
 
   private static final Attributes.Key<String> ADDITIONAL_TRANSPORT_ATTR_KEY =
-      Attributes.Key.of("additional-attr");
+      Attributes.Key.create("additional-attr");
 
   /**
    * Returns a new server that when started will be able to be connected to from the client. Each