fixed method names changes (#3)

* fixed method names changes

* refactored code based on feedback

* added logger & precondition

* fixed names & error messages

Author: rahulKQL

bigtable-client-core-parent/bigtable-hbase/src/main/java/com/google/cloud/bigtable/hbase/BigtableDataSettingsFactory.java

@@ -15,6 +15,7 @@
  */
 package com.google.cloud.bigtable.hbase;
 
+import static com.google.api.client.util.Preconditions.checkState;
 import static org.threeten.bp.Duration.ofMillis;
 
 import java.io.FileInputStream;
@@ -32,7 +33,6 @@
 import com.google.api.gax.core.NoCredentialsProvider;
 import com.google.api.gax.grpc.GrpcTransportChannel;
 import com.google.api.gax.retrying.RetrySettings;
-import com.google.api.gax.rpc.FixedHeaderProvider;
 import com.google.api.gax.rpc.FixedTransportChannelProvider;
 import com.google.cloud.bigtable.config.BigtableOptions;
 import com.google.cloud.bigtable.config.BulkOptions;
@@ -46,7 +46,6 @@
 import com.google.cloud.bigtable.data.v2.stub.BigtableStubSettings;
 
 import io.grpc.ManagedChannelBuilder;
-import io.grpc.internal.GrpcUtil;
 
 /**
  * Static methods to convert an instance of {@link Configuration} or {@link BigtableOptions} to a
@@ -65,49 +64,40 @@ public class BigtableDataSettingsFactory {
    */
   public static BigtableDataSettings fromBigtableOptions(final BigtableOptions options)
       throws IOException, GeneralSecurityException {
-    if (!options.getRetryOptions().enableRetries()) {
-      throw new IllegalStateException(
-          "Retry is must for BigtableDataSettings configuration from BigtableOptions.");
-    }
-
+    checkState(!options.getRetryOptions().enableRetries(), "Disabling retries is not currently supported.");
+    
     BigtableDataSettings.Builder builder = BigtableDataSettings.newBuilder();
 
     InstanceName instanceName = InstanceName.newBuilder().setProject(options.getProjectId())
         .setInstance(options.getInstanceId()).build();
     builder.setInstanceName(instanceName);
     builder.setAppProfileId(options.getAppProfileId());
 
-    LOG.debug("endpoint host %s.", options.getDataHost());
-    LOG.debug("endpoint host %s.", options.getPort());
     builder.setEndpoint(options.getDataHost() + ":" + options.getPort());
 
-    buildCredentialProvider(builder, options.getCredentialOptions());
-
-    buildBulkOptions(builder, options);
+    buildCredentialProviderSettings(builder, options.getCredentialOptions());
 
-    buildCheckAndMutateRow(builder, options.getCallOptionsConfig().getShortRpcTimeoutMs());
+    buildBulkMutationsSettings(builder, options);
 
-    buildReadModifyWrite(builder, options.getCallOptionsConfig().getShortRpcTimeoutMs());
+    buildCheckAndMutateRowSettings(builder, options.getCallOptionsConfig().getShortRpcTimeoutMs());
 
-    buildReadRows(builder, options);
+    buildReadModifyWriteSettings(builder, options.getCallOptionsConfig().getShortRpcTimeoutMs());
 
-    buildMutateRow(builder, options);
+    buildReadRowsSettings(builder, options);
 
-    buildSampleRowKeys(builder, options);
+    buildMutateRowSettings(builder, options);
 
-    // TODO: would it map to GrpcHeaderInterceptor? or we should build userAgent
-    // using ManagedChannelBuilder AND
-    builder.setHeaderProvider(
-      FixedHeaderProvider.create(GrpcUtil.USER_AGENT_KEY.name(), options.getUserAgent()));
+    buildSampleRowKeysSettings(builder, options);
 
     // TODO: implementation for channelCount or channelPerCPU
     ManagedChannelBuilder channelBuilder = ManagedChannelBuilder
-        .forAddress(options.getDataHost(), options.getPort())//
+        .forAddress(options.getDataHost(), options.getPort())
         .userAgent(options.getUserAgent());
 
     if (options.usePlaintextNegotiation()) {
       channelBuilder.usePlaintext();
     }
+
     builder.setTransportChannelProvider(
       FixedTransportChannelProvider.create(GrpcTransportChannel.create(channelBuilder.build())));
 
@@ -120,23 +110,22 @@ public static BigtableDataSettings fromBigtableOptions(final BigtableOptions opt
    * @param builder a {@link BigtableDataSettings.Builder} object.
    * @param options a {@link BigtableOptions} object.
    */
-  private static void buildBulkOptions(Builder builder, BigtableOptions options) {
+  private static void buildBulkMutationsSettings(Builder builder, BigtableOptions options) {
     BulkOptions bulkOptions = options.getBulkOptions();
     BatchingSettings.Builder batchSettingsBuilder = BatchingSettings.newBuilder();
 
-    FlowControlSettings.Builder flowControlBuilder =
-        FlowControlSettings.newBuilder()
-            .setMaxOutstandingRequestBytes(bulkOptions.getMaxMemory());
-
     long autoFlushMs = bulkOptions.getAutoflushMs();
     long bulkMaxRowKeyCount = bulkOptions.getBulkMaxRowKeyCount();
     long maxInflightRpcs = bulkOptions.getMaxInflightRpcs();
 
     if (autoFlushMs > 0) {
       batchSettingsBuilder.setDelayThreshold(Duration.ofMillis(autoFlushMs));
     }
+    FlowControlSettings.Builder flowControlBuilder = FlowControlSettings.newBuilder();
     if (maxInflightRpcs > 0) {
-      flowControlBuilder.setMaxOutstandingElementCount(maxInflightRpcs * bulkMaxRowKeyCount);
+      flowControlBuilder
+        .setMaxOutstandingRequestBytes(bulkOptions.getMaxMemory())
+        .setMaxOutstandingElementCount(maxInflightRpcs * bulkMaxRowKeyCount);
     }
 
     batchSettingsBuilder
@@ -156,9 +145,9 @@ private static void buildBulkOptions(Builder builder, BigtableOptions options) {
    * @param builder a {@link BigtableDataSettings.Builder} object.
    * @param bulkMutation a {@link BulkOptions} object.
    */
-  private static void buildSampleRowKeys(Builder builder, BigtableOptions options) {
+  private static void buildSampleRowKeysSettings(Builder builder, BigtableOptions options) {
     builder.sampleRowKeysSettings()
-        .setRetrySettings(defaultRetrySettings(options));
+        .setRetrySettings(buildIdempotentRetrySettings(options));
   }
 
   /**
@@ -167,9 +156,9 @@ private static void buildSampleRowKeys(Builder builder, BigtableOptions options)
    * @param builder a {@link BigtableDataSettings.Builder} object.
    * @param bulkMutation a {@link BulkOptions} object.
    */
-  private static void buildMutateRow(Builder builder, BigtableOptions options) {
+  private static void buildMutateRowSettings(Builder builder, BigtableOptions options) {
     builder.mutateRowSettings()
-        .setRetrySettings(defaultRetrySettings(options));
+        .setRetrySettings(buildIdempotentRetrySettings(options));
   }
 
   /**
@@ -178,10 +167,24 @@ private static void buildMutateRow(Builder builder, BigtableOptions options) {
    * @param builder a {@link BigtableDataSettings.Builder} object.
    * @param bulkMutation a {@link BulkOptions} object.
    */
-  private static void buildReadRows(Builder builder, BigtableOptions options) {
-    // TODO: set readPartialRowTimeout for watchdog timer, taken BigtableSession#setupWatchdog()
+  private static void buildReadRowsSettings(Builder builder, BigtableOptions options) {
+    RetryOptions retryOptions = options.getRetryOptions();
+
+    RetrySettings.Builder retryBuilder = RetrySettings.newBuilder()
+        .setInitialRetryDelay(ofMillis(retryOptions.getInitialBackoffMillis()))
+        .setRetryDelayMultiplier(retryOptions.getBackoffMultiplier())
+        .setMaxRetryDelay(ofMillis(retryOptions.getMaxElapsedBackoffMillis()))
+        .setMaxAttempts(retryOptions.getMaxScanTimeoutRetries());
+
+    // configurations for RPC timeouts
+    Duration readPartialRowTimeout = ofMillis(retryOptions.getReadPartialRowTimeoutMillis());
+    retryBuilder
+        .setInitialRpcTimeout(readPartialRowTimeout)
+        .setMaxRpcTimeout(readPartialRowTimeout)
+        .setTotalTimeout(ofMillis(options.getCallOptionsConfig().getLongRpcTimeoutMs()));
+
     builder.readRowsSettings()
-        .setRetrySettings(defaultRetrySettings(options));
+        .setRetrySettings(retryBuilder.build());
   }
 
   /**
@@ -190,7 +193,7 @@ private static void buildReadRows(Builder builder, BigtableOptions options) {
    * @param builder a {@link BigtableDataSettings.Builder} object.
    * @param bulkMutation a {@link BulkOptions} object.
    */
-  private static void buildReadModifyWrite(Builder builder, long rpcTimeoutMs) {
+  private static void buildReadModifyWriteSettings(Builder builder, long rpcTimeoutMs) {
     builder.readModifyWriteRowSettings()
         .setSimpleTimeoutNoRetries(ofMillis(rpcTimeoutMs));
   }
@@ -201,7 +204,7 @@ private static void buildReadModifyWrite(Builder builder, long rpcTimeoutMs) {
    * @param builder a {@link BigtableDataSettings.Builder} object.
    * @param bulkMutation a {@link BulkOptions} object.
    */
-  private static void buildCheckAndMutateRow(Builder builder, long rpcTimeoutMs) {
+  private static void buildCheckAndMutateRowSettings(Builder builder, long rpcTimeoutMs) {
     builder.checkAndMutateRowSettings()
         .setSimpleTimeoutNoRetries(ofMillis(rpcTimeoutMs));
   }
@@ -212,7 +215,7 @@ private static void buildCheckAndMutateRow(Builder builder, long rpcTimeoutMs) {
    * @param builder a {@link BigtableDataSettings.Builder} object.
    * @param bulkMutation a {@link BulkOptions} object.
    */
-  private static RetrySettings defaultRetrySettings(BigtableOptions options) {
+  private static RetrySettings buildIdempotentRetrySettings(BigtableOptions options) {
     RetryOptions retryOptions = options.getRetryOptions();
 
     RetrySettings.Builder retryBuilder = RetrySettings.newBuilder()
@@ -222,12 +225,16 @@ private static RetrySettings defaultRetrySettings(BigtableOptions options) {
         .setMaxAttempts(retryOptions.getMaxScanTimeoutRetries());
 
     // configurations for RPC timeouts
+    Duration shortRpcTimeout = ofMillis(options.getCallOptionsConfig().getShortRpcTimeoutMs());
     retryBuilder
-        .setInitialRpcTimeout(ofMillis(options.getCallOptionsConfig().getShortRpcTimeoutMs()))
-        .setMaxRpcTimeout(ofMillis(retryOptions.getReadPartialRowTimeoutMillis()))
+        .setInitialRpcTimeout(shortRpcTimeout)
+        .setMaxRpcTimeout(shortRpcTimeout)
         .setTotalTimeout(ofMillis(options.getCallOptionsConfig().getLongRpcTimeoutMs()));
-
-    // TODO: an option to set RetryOptions#allowRetriesWithoutTimestamp
+    
+    if (retryOptions.allowRetriesWithoutTimestamp()) {
+      //TODO: add instruction to create unsafeMutation.
+      LOG.warn("Retries without Timestamp doesn't support");
+    }
     return retryBuilder.build();
   }
 
@@ -239,7 +246,7 @@ private static RetrySettings defaultRetrySettings(BigtableOptions options) {
    * @throws FileNotFoundException
    * @throws IOException
    */
-  private static void buildCredentialProvider(Builder builder, CredentialOptions credentialOptions)
+  private static void buildCredentialProviderSettings(Builder builder, CredentialOptions credentialOptions)
       throws FileNotFoundException, IOException {
     CredentialsProvider credentialsProvider = NoCredentialsProvider.create();
 

bigtable-client-core-parent/bigtable-hbase/src/test/java/com/google/cloud/bigtable/hbase/TestBigtableDataSettingsFactory.java

@@ -58,11 +58,14 @@ public void testInstanceIdIsRequired() throws IOException, GeneralSecurityExcept
     BigtableDataSettingsFactory.fromBigtableOptions(options);
   }
 
-  @Test(expected = NullPointerException.class)
+  @Test
   public void testWithoutUserAgent() throws IOException, GeneralSecurityException {
     BigtableOptions options = BigtableOptions.builder().setProjectId(TEST_PROJECT_ID)
         .setInstanceId(TEST_INSTANCE_ID).build();
-    BigtableDataSettingsFactory.fromBigtableOptions(options);
+    BigtableDataSettings dataSettings = BigtableDataSettingsFactory.fromBigtableOptions(options);
+    // TODO: Need to assert UserAgent to null & add more test cases
+    Assert.assertEquals(TEST_PROJECT_ID, dataSettings.getInstanceName().getProject());
+    Assert.assertEquals(TEST_INSTANCE_ID, dataSettings.getInstanceName().getInstance());
   }
 
   @Test