Third round of updates to address PR comments

* ApiUtils: Fixing usage of credentials when channel is null
* ApiUtils: Function rename for clarity
* ApiUtils, ServiceApiSettings: improving comments
* ApiCallable: Removing obsolete stuff
* not returning 'this' from LocalPubsubHelper.start()

Author: garrettjonesgoogle

gcloud-java-gax/src/main/java/io/gapi/gax/grpc/ApiCallable.java

@@ -62,8 +62,6 @@
 public abstract class ApiCallable<RequestT, ResponseT> {
 
   // TODO(wrwg): Support interceptors and method/call option configurations.
-  // TODO(wrwg): gather more feedback whether the overload with java.util.Concurrent hurts that
-  //             much that we want to rename this into ClientCallable or such.
 
   // Subclass Contract
   // =================
@@ -390,20 +388,4 @@ public ApiCallable<RequestT, ResponseT> retrying() {
     return new PageStreamingCallable<RequestT, ResponseT, ResourceT>(this, pageDescriptor);
   }
 
-  /**
-   * Returns a callable which behaves the same as {@link #pageStreaming(PageDescriptor)}, with
-   * the page descriptor attempted to derive from the callable descriptor.
-   *
-   * @throws IllegalArgumentException if a page descriptor is not derivable.
-   */
-  public <ResourceT> ApiCallable<RequestT, ResourceT>
-      pageStreaming(Class<ResourceT> resourceType) {
-    PageDescriptor<RequestT, ResponseT, ResourceT> pageDescriptor =
-        getDescriptor() != null ? getDescriptor().getPageDescriptor(resourceType) : null;
-    if (pageDescriptor == null) {
-      throw new IllegalArgumentException(String.format(
-          "cannot derive page descriptor for '%s'", this));
-    }
-    return pageStreaming(pageDescriptor);
-  }
 }

gcloud-java-gax/src/main/java/io/gapi/gax/grpc/ServiceApiSettings.java

@@ -35,20 +35,23 @@
 
 import io.grpc.ManagedChannel;
 
+/**
+ * A settings class to configure a service api class.
+ */
 public class ServiceApiSettings {
   private boolean isIdempotentRetrying;
 
   private Credentials credentials;
 
-  private String servicePath;
+  private String serviceAddress;
   private int port;
 
   private ManagedChannel channel;
 
   public ServiceApiSettings() {
     isIdempotentRetrying = true;
     credentials = null;
-    servicePath = null;
+    serviceAddress = null;
     port = 0;
   }
 
@@ -69,7 +72,8 @@ public boolean getIsIdempotentRetrying() {
 
   /**
    * Sets the credentials to use in order to call the service. The default is to acquire
-   * the credentials using GoogleCredentials.getApplicationDefault().
+   * the credentials using GoogleCredentials.getApplicationDefault(). These credentials
+   * will not be used if the channel is set.
    */
   public ServiceApiSettings setCredentials(Credentials credentials) {
     this.credentials = credentials;
@@ -81,19 +85,19 @@ public Credentials getCredentials() {
   }
 
   /**
-   * The path used to reach the service.
+   * The path used to reach the service. This value will not be used if the channel is set.
    */
-  public ServiceApiSettings setServicePath(String servicePath) {
-    this.servicePath = servicePath;
+  public ServiceApiSettings setServiceAddress(String serviceAddress) {
+    this.serviceAddress = serviceAddress;
     return this;
   }
 
-  public String getServicePath() {
-    return servicePath;
+  public String getServiceAddress() {
+    return serviceAddress;
   }
 
   /**
-   * The port used to reach the service.
+   * The port used to reach the service. This value will not be used if the channel is set.
    */
   public ServiceApiSettings setPort(int port) {
     this.port = port;

gcloud-java-gax/src/main/java/io/gapi/gax/internal/ApiUtils.java

@@ -31,12 +31,7 @@
 
 package io.gapi.gax.internal;
 
-import java.io.IOException;
-import java.util.Arrays;
-import java.util.Collection;
-import java.util.List;
-import java.util.concurrent.Executors;
-
+import com.google.auth.Credentials;
 import com.google.auth.oauth2.GoogleCredentials;
 import com.google.common.collect.Lists;
 
@@ -48,6 +43,11 @@
 import io.grpc.netty.NegotiationType;
 import io.grpc.netty.NettyChannelBuilder;
 
+import java.io.IOException;
+import java.util.Arrays;
+import java.util.List;
+import java.util.concurrent.Executors;
+
 public class ApiUtils {
 
   // TODO(wgg): make this configurable
@@ -63,17 +63,26 @@ public static <RequestT, ResponseT> ApiCallable<RequestT, ResponseT> prepareIdem
   }
 
   /**
-   * Creates a channel for the given path, address and port.
+   * Acquires application-default credentials, applying the given scopes if the
+   * credentials require scopes.
+   */
+  public static Credentials credentialsWithScopes(String scopes[]) throws IOException {
+    List<String> scopeList = Arrays.asList(scopes);
+    GoogleCredentials credentials = GoogleCredentials.getApplicationDefault();
+    if (credentials.createScopedRequired()) {
+      credentials = credentials.createScoped(scopeList);
+    }
+    return credentials;
+  }
+
+  /**
+   * Creates a channel for the given address, port, and credentials.
    */
-  public static ManagedChannel createChannel(String address, int port, Collection<String> scopes)
+  public static ManagedChannel createChannel(String address, int port, Credentials credentials)
       throws IOException {
     List<ClientInterceptor> interceptors = Lists.newArrayList();
     //TODO: MIGRATION interceptors.add(ChannelFactory.authorityInterceptor(address));
 
-    GoogleCredentials credentials = GoogleCredentials.getApplicationDefault();
-    if (credentials.createScopedRequired()) {
-      credentials = credentials.createScoped(scopes);
-    }
     interceptors.add(new ClientAuthInterceptor(credentials,
         Executors.newFixedThreadPool(AUTH_THREADS)));
 
@@ -84,23 +93,31 @@ public static ManagedChannel createChannel(String address, int port, Collection<
         .build();
   }
 
-  public static ServiceApiSettings settingsWithChannels(ServiceApiSettings settings,
-      String defaultServicePath, int defaultServicePort, String scopes[]) throws IOException {
+  /**
+   * Creates a new instance of ServiceApiSettings with all fields populated, using
+   * the given defaults if the corresponding values are not set on ServiceApiSettings.
+   */
+  public static ServiceApiSettings populateSettings(ServiceApiSettings settings,
+      String defaultServiceAddress, int defaultServicePort, String scopes[]) throws IOException {
     ManagedChannel channel = settings.getChannel();
 
     if (channel == null) {
-      String servicePath = defaultServicePath;
-      if (settings.getServicePath() != null) {
-        servicePath = settings.getServicePath();
+      String servicePath = settings.getServiceAddress();
+      if (servicePath == null) {
+        servicePath = defaultServiceAddress;
+      }
+
+      int port = settings.getPort();
+      if (port == 0) {
+        port = defaultServicePort;
       }
 
-      int port = defaultServicePort;
-      if (settings.getPort() != 0) {
-        port = settings.getPort();
+      Credentials credentials = settings.getCredentials();
+      if (credentials == null) {
+        credentials = credentialsWithScopes(scopes);
       }
 
-      List<String> scopeList = Arrays.asList(scopes);
-      channel = ApiUtils.createChannel(servicePath, port, scopeList);
+      channel = ApiUtils.createChannel(servicePath, port, credentials);
     }
 
     return new ServiceApiSettings()

gcloud-java-pubsub/src/main/java/com/google/gcloud/pubsub/spi/PublisherApi.java

@@ -191,7 +191,7 @@ public static PublisherApi create(ServiceApiSettings settings) throws IOExceptio
    * easy to make a subclass, but otherwise, the static factory methods should be preferred.
    */
   protected PublisherApi(ServiceApiSettings settings) throws IOException {
-    ServiceApiSettings internalSettings = ApiUtils.settingsWithChannels(settings,
+    ServiceApiSettings internalSettings = ApiUtils.populateSettings(settings,
         SERVICE_ADDRESS, DEFAULT_SERVICE_PORT, ALL_SCOPES);
     this.settings = internalSettings;
     this.channel = internalSettings.getChannel();

gcloud-java-pubsub/src/main/java/com/google/gcloud/pubsub/spi/SubscriberApi.java

@@ -172,7 +172,7 @@ public static SubscriberApi create(ServiceApiSettings settings) throws IOExcepti
    * easy to make a subclass, but otherwise, the static factory methods should be preferred.
    */
   protected SubscriberApi(ServiceApiSettings settings) throws IOException {
-    ServiceApiSettings internalSettings = ApiUtils.settingsWithChannels(settings,
+    ServiceApiSettings internalSettings = ApiUtils.populateSettings(settings,
         SERVICE_ADDRESS, DEFAULT_SERVICE_PORT, ALL_SCOPES);
     this.settings = internalSettings;
     this.channel = internalSettings.getChannel();

gcloud-java-pubsub/src/main/java/com/google/gcloud/pubsub/testing/LocalPubsubHelper.java

@@ -43,13 +43,12 @@ public LocalPubsubHelper(String addressString) {
   /**
    * Starts the in-memory service.
    */
-  public LocalPubsubHelper start() {
+  public void start() {
     try {
       server.start();
     } catch (IOException ex) {
       throw new RuntimeException(ex);
     }
-    return this;
   }
 
   /**

gcloud-java-pubsub/src/test/java/com/google/gcloud/pubsub/spi/PublisherApiTest.java

@@ -38,7 +38,8 @@ public class PublisherApiTest {
 
   @BeforeClass
   public static void startStaticServer() {
-    pubsubHelper = new LocalPubsubHelper("in-process-1").start();
+    pubsubHelper = new LocalPubsubHelper("in-process-1");
+    pubsubHelper.start();
   }
 
   @AfterClass