code review updates.

google-cloud-clients/google-cloud-bigtable/src/main/java/com/google/cloud/bigtable/admin/v2/models/GCRules.java

@@ -71,9 +71,7 @@ public VersionRule maxVersions(int maxVersion) {
    * @return DurationRule
    */
   public DurationRule maxAge(long maxAge, TimeUnit timeUnit) {
-    Duration duration = Duration.ZERO;
-    TimeUnit.SECONDS.convert(maxAge, timeUnit);
-    return maxAge(duration);
+    return maxAge(Duration.ofNanos(TimeUnit.NANOSECONDS.convert(maxAge, timeUnit)));
   }
 
   /**
@@ -100,11 +98,10 @@ public DefaultRule defaulRule() {
    * intersection as the root
    */
   public static final class IntersectionRule extends BaseRule {
-    private GcRule.Intersection.Builder builder;
-    private List<GCRule> rulesList = new ArrayList<>();
+    private final List<GCRule> rulesList;
 
     private IntersectionRule() {
-      this.builder = GcRule.Intersection.newBuilder();
+      rulesList = new ArrayList<>();
     }
 
     /**
@@ -115,7 +112,6 @@ private IntersectionRule() {
      */
     public IntersectionRule rule(@Nonnull GCRule rule) {
       rulesList.add(rule);
-      builder.addRules(rule.toProto());
       return this;
     }
 
@@ -136,13 +132,16 @@ public String toString() {
     @InternalApi
     @Override
     public GcRule toProto() {
-      switch (builder.getRulesCount()) {
+      switch (rulesList.size()) {
         case 0:
           return GcRule.newBuilder().build();
         case 1:
-          return builder.getRules(0);
+          return rulesList.get(0).toProto();
         default:
-          return GcRule.newBuilder().setIntersection(builder.build()).build();
+          return GcRule.newBuilder()
+                       .setIntersection(
+                           Intersection.newBuilder().addAllRules(convertToGcRules(rulesList)))
+                       .build();
       }
     }
   }
@@ -152,11 +151,10 @@ public GcRule toProto() {
    * the root
    */
   public static final class UnionRule extends BaseRule {
-    private GcRule.Union.Builder builder;
-    private List<GCRule> rulesList = new ArrayList<>();
+    private final List<GCRule> rulesList;
 
     private UnionRule() {
-      this.builder = GcRule.Union.newBuilder();
+      rulesList = new ArrayList<>();
     }
 
     /**
@@ -167,7 +165,6 @@ private UnionRule() {
      */
     public UnionRule rule(@Nonnull GCRule rule) {
       rulesList.add(rule);
-      builder.addRules(rule.toProto());
       return this;
     }
 
@@ -188,20 +185,23 @@ public String toString() {
     @InternalApi
     @Override
     public GcRule toProto() {
-      switch (builder.getRulesCount()) {
+      switch (rulesList.size()) {
         case 0:
           return GcRule.newBuilder().build();
         case 1:
-          return builder.getRules(0);
+          return rulesList.get(0).toProto();
         default:
-          return GcRule.newBuilder().setUnion(builder.build()).build();
+          return GcRule.newBuilder()
+              .setUnion(
+                  Union.newBuilder().addAllRules(convertToGcRules(rulesList)))
+              .build();
       }
     }
   }
 
   /** Wrapper for building max versions rule */
   public static final class VersionRule extends BaseRule {
-    private GcRule.Builder builder;
+    private final GcRule.Builder builder;
 
     private VersionRule(int maxVersion) {
       this.builder = GcRule.newBuilder();
@@ -227,7 +227,7 @@ public GcRule toProto() {
 
   /** Wrapper for building max duration rule */
   public static final class DurationRule extends BaseRule {
-    private com.google.protobuf.Duration.Builder builder;
+    private final com.google.protobuf.Duration.Builder builder;
 
     private DurationRule(Duration duration) {
       this.builder =
@@ -312,4 +312,13 @@ public interface GCRule {
     @InternalApi
     GcRule toProto();
   }
+
+  private static List<GcRule> convertToGcRules(List<GCRule> rules) {
+    List<GcRule> gcRules = new ArrayList<>(rules.size());
+
+    for (GCRule rule : rules) {
+      gcRules.add(rule.toProto());
+    }
+    return gcRules;
+  }
 }

google-cloud-clients/google-cloud-bigtable/src/main/java/com/google/cloud/bigtable/admin/v2/models/TableAdminRequests.java

@@ -19,14 +19,13 @@
 import com.google.api.core.InternalApi;
 import com.google.bigtable.admin.v2.ColumnFamily;
 import com.google.bigtable.admin.v2.CreateTableRequest;
-import com.google.bigtable.admin.v2.Table;
-import com.google.bigtable.admin.v2.TableName;
-import com.google.bigtable.admin.v2.Table.TimestampGranularity;
 import com.google.bigtable.admin.v2.CreateTableRequest.Split;
-import com.google.bigtable.admin.v2.GcRule;
 import com.google.bigtable.admin.v2.InstanceName;
 import com.google.bigtable.admin.v2.ModifyColumnFamiliesRequest;
 import com.google.bigtable.admin.v2.ModifyColumnFamiliesRequest.Modification;
+import com.google.bigtable.admin.v2.Table;
+import com.google.bigtable.admin.v2.Table.TimestampGranularity;
+import com.google.bigtable.admin.v2.TableName;
 import com.google.cloud.bigtable.admin.v2.models.GCRules.GCRule;
 import com.google.common.base.Preconditions;
 import com.google.protobuf.ByteString;
@@ -59,12 +58,12 @@ public static ModifyFamilies modifyFamilies(String tableId) {
   /**
    * Fluent wrapper for {@link CreateTableRequest}
    *
-   * <pre>
-   * Allows for creating table with
-   *    - optional columnFamilies, including optional {@link GCRule}
-   *    - optional granularity
-   *    - and optional split points
-   * </pre>
+   * <p> Allows for creating table with:
+   * <ul>
+   * <li> optional columnFamilies, including optional {@link GCRule}
+   * <li> optional granularity
+   * <li> and optional split points
+   * </ul>
    */
   public static final class CreateTable {
     private final CreateTableRequest.Builder createTableRequest = CreateTableRequest.newBuilder();
@@ -150,17 +149,17 @@ public CreateTableRequest toProto(InstanceName instanceName) {
   /**
    * Fluent wrapper for {@link ModifyColumnFamiliesRequest}
    *
-   * <pre>
-   * Allows the following ColumnFamily modifications
-   *    - create family, optionally with {@link GCRule}
-   *    - update existing family {@link GCRule}
-   *    - drop an existing family
-   * </pre>
+   * <p> Allows for the following ColumnFamily modifications:
+   * <ul> 
+   * <li> create family, optionally with {@link GCRule}
+   * <li> update existing family {@link GCRule}
+   * <li> drop an existing family
+   * </ul>
    */
   public static final class ModifyFamilies {
     private final ModifyColumnFamiliesRequest.Builder modFamilyRequest =
         ModifyColumnFamiliesRequest.newBuilder();
-    private String tableId;
+    private final String tableId;
 
     /**
      * Configures the tableId to execute the modifications
@@ -179,7 +178,7 @@ private ModifyFamilies(String tableId) {
      * @return
      */
     public ModifyFamilies create(String familyId) {
-      return createWithGCRule(familyId, null);
+      return create(familyId, GCRules.GCRULES.defaulRule());
     }
 
     /**
@@ -189,10 +188,10 @@ public ModifyFamilies create(String familyId) {
      * @param gcRule
      * @return
      */
-    public ModifyFamilies createWithGCRule(String familyId, GCRule gcRule) {
+    public ModifyFamilies create(String familyId, GCRule gcRule) {
       Modification.Builder modification = Modification.newBuilder().setId(familyId);
-      GcRule grule = (gcRule == null) ? GcRule.getDefaultInstance() : gcRule.toProto();
-      modification.setCreate(ColumnFamily.newBuilder().setGcRule(grule));
+      Preconditions.checkNotNull(gcRule);
+      modification.setCreate(ColumnFamily.newBuilder().setGcRule(gcRule.toProto()));
       modFamilyRequest.addModifications(modification.build());
       return this;
     }
@@ -204,7 +203,7 @@ public ModifyFamilies createWithGCRule(String familyId, GCRule gcRule) {
      * @param gcRule
      * @return
      */
-    public ModifyFamilies updateWithGCRule(String familyId, GCRule gcRule) {
+    public ModifyFamilies update(String familyId, GCRule gcRule) {
       Modification.Builder modification = Modification.newBuilder().setId(familyId);
       Preconditions.checkNotNull(gcRule);
       modification.setUpdate(ColumnFamily.newBuilder().setGcRule(gcRule.toProto()));

google-cloud-clients/google-cloud-bigtable/src/main/java/com/google/cloud/bigtable/admin/v2/models/TableAdminResponses.java

@@ -46,7 +46,7 @@ private TableAdminResponses() {}
   /**
    * Converts the protocol buffer table to a simpler Table model with only the required elements
    *
-   * @param com.google.bigtable.admin.v2.Table - Protobuf table
+   * @param table - Protobuf table
    * @return Table - Table response wrapper
    */
   @InternalApi
@@ -58,39 +58,62 @@ public static Table convertTable(com.google.bigtable.admin.v2.Table table) {
    * Converts the protocol buffer response to a simpler ConsistencyToken which can be passed back as
    * is.
    *
-   * @param GenerateConsistencyTokenResponse - Protobuf ConsistencyTokenResponse
+   * @param tokenResponse - Protobuf ConsistencyTokenResponse
    * @return ConsistencyToken - ConsistencyToken response wrapper
    */
+  @InternalApi
   public static ConsistencyToken convertTokenResponse(
       GenerateConsistencyTokenResponse tokenResponse) {
     return new ConsistencyToken(tokenResponse);
   }
 
+  /**
+   * Converts the protocol buffer response to a simpler ClusterState model with only required elements
+   * 
+   * @param clusterStatesMap - Protobuf clusterStatesMap
+   * @return Map<String, ClusterState> 
+   */
+  @InternalApi
+  public static Map<String, ClusterState> convertClusterStates(
+      Map<String, com.google.bigtable.admin.v2.Table.ClusterState> clusterStatesMap) {
+    Map<String, ClusterState> clusterStates = new HashMap<>();
+
+    for (Entry<String, com.google.bigtable.admin.v2.Table.ClusterState> entry : clusterStatesMap.entrySet()) {
+      clusterStates.put(entry.getKey(), new ClusterState(entry.getKey(), entry.getValue()));
+    }
+    return clusterStates;
+  }
+
+  /**
+   * Converts the protocol buffer response to a simpler ColumnFamily model with only required elements
+   * 
+   * @param columnFamiliesMap - Protobuf columnFamiliesMap
+   * @return Map<String, ColumnFamily>
+   */
+  @InternalApi
+  public static Map<String, ColumnFamily> convertColumnFamilies(
+      Map<String, com.google.bigtable.admin.v2.ColumnFamily> columnFamiliesMap) {
+    Map<String, ColumnFamily> columnFamilies = new HashMap<>();
+
+    for (Entry<String, com.google.bigtable.admin.v2.ColumnFamily> entry : columnFamiliesMap.entrySet()) {
+      columnFamilies.put(entry.getKey(), new ColumnFamily(entry.getKey(), entry.getValue()));
+    }
+    return columnFamilies;
+  }
+
   /** Wrapper for {@link Table} protocol buffer object */
   public static final class Table {
-    private TableName tableName;
-    private TimestampGranularity timestampGranularity;
-    private Map<String, ClusterState> clusterStates = new HashMap<>();
-    private Map<String, ColumnFamily> columnFamilies = new HashMap<>();
+    private final TableName tableName;
+    private final TimestampGranularity timestampGranularity;
+    private final Map<String, ClusterState> clusterStates;
+    private final Map<String, ColumnFamily> columnFamilies;
 
     private Table(com.google.bigtable.admin.v2.Table table) {
       Preconditions.checkNotNull(table);
       this.tableName = TableName.parse(table.getName());
       this.timestampGranularity = table.getGranularity();
-
-      Map<String, com.google.bigtable.admin.v2.Table.ClusterState> clusterStatesMap =
-          table.getClusterStatesMap();
-      for (Entry<String, com.google.bigtable.admin.v2.Table.ClusterState> entry :
-          clusterStatesMap.entrySet()) {
-        clusterStates.put(entry.getKey(), new ClusterState(entry.getKey(), entry.getValue()));
-      }
-
-      Map<String, com.google.bigtable.admin.v2.ColumnFamily> columnFamiliesMap =
-          table.getColumnFamiliesMap();
-      for (Entry<String, com.google.bigtable.admin.v2.ColumnFamily> entry :
-          columnFamiliesMap.entrySet()) {
-        columnFamilies.put(entry.getKey(), new ColumnFamily(entry.getKey(), entry.getValue()));
-      }
+      this.clusterStates = convertClusterStates(table.getClusterStatesMap());
+      this.columnFamilies = convertColumnFamilies(table.getColumnFamiliesMap());
     }
 
     /**
@@ -162,8 +185,8 @@ public String toString() {
 
   /** Wrapper for {@link ClusterState} protocol buffer object */
   public static final class ClusterState {
-    private String id;
-    private ReplicationState replicationState;
+    private final String id;
+    private final ReplicationState replicationState;
 
     private ClusterState(String id, com.google.bigtable.admin.v2.Table.ClusterState clusterState) {
       this.id = id;
@@ -280,7 +303,7 @@ private GCRule convertGcRule(GcRule source) {
    * They are obtained by invoking {@link TableAdminClient#generateConsistencyToken(String)}
    */
   public static final class ConsistencyToken {
-    private String token;
+    private final String token;
 
     private ConsistencyToken(GenerateConsistencyTokenResponse resp) {
       this.token = resp.getConsistencyToken();

google-cloud-clients/google-cloud-bigtable/src/test/java/com/google/cloud/bigtable/admin/v2/TableAdminClientTest.java

@@ -212,7 +212,7 @@ public void getDropRowRangeRequest() {
   }
 
   @Test
-  public void getDropRowRangeRequest_dropAllData() {
+  public void getDropRowRangeRequestDropAllData() {
     DropRowRangeRequest actual = adminClient.composeDropRowRangeRequest("tableId", null, true);
 
     DropRowRangeRequest expected =

google-cloud-clients/google-cloud-bigtable/src/test/java/com/google/cloud/bigtable/admin/v2/it/TableAdminClientIT.java

@@ -41,12 +41,12 @@ public class TableAdminClientIT {
   static TableAdminClient tableAdmin;
 
   @BeforeClass
-  public static void setup() throws IOException {
+  public static void setUp() throws IOException {
     tableAdmin = TableAdminClient.create(InstanceName.of("sduskis-hello-shakespear", "beam-test"));
   }
 
   @AfterClass
-  public static void cleanup() throws Exception {
+  public static void cleanUp() throws Exception {
     tableAdmin.close();
   }
 
@@ -90,20 +90,20 @@ public void modifyFamilies() {
     Duration.ofSeconds(1000);
     modifyFamiliesReq
         .create("mf1")
-        .createWithGCRule("mf2", GCRULES.maxAge(Duration.ofSeconds(1000, 20000)))
-        .updateWithGCRule(
+        .create("mf2", GCRULES.maxAge(Duration.ofSeconds(1000, 20000)))
+        .update(
             "mf1",
             GCRULES
                 .union()
                 .rule(GCRULES.maxAge(Duration.ofSeconds(100)))
                 .rule(GCRULES.maxVersions(1)))
-        .createWithGCRule(
+        .create(
             "mf3",
             GCRULES
                 .intersection()
                 .rule(GCRULES.maxAge(Duration.ofSeconds(2000)))
                 .rule(GCRULES.maxVersions(10)))
-        .createWithGCRule(
+        .create(
             "mf4", GCRULES.intersection().rule(GCRULES.maxAge(Duration.ofSeconds(360))))
         .create("mf5")
         .create("mf6")
@@ -224,7 +224,7 @@ public void dropRowRange() {
     try {
       tableAdmin.createTable(TableAdminRequests.createTable(tableId));
       tableAdmin.dropRowRange(tableId, "rowPrefix");
-      tableAdmin.dropAllData(tableId);
+      tableAdmin.dropAllRows(tableId);
     } finally {
       tableAdmin.deleteTable(tableId);
     }