[GitHub] [accumulo] keith-turner commented on a change in pull request #1651: #1473 Move TabletsMetadata builder to Ample

[GitBox <gi...@apache.org>]

keith-turner commented on a change in pull request #1651:
URL: https://github.com/apache/accumulo/pull/1651#discussion_r493660218



##########
File path: test/src/main/java/org/apache/accumulo/test/functional/AccumuloClientIT.java
##########
@@ -220,6 +238,120 @@ public void testClose() throws Exception {
     expectClosed(() -> tops.create("expectFail"));
     expectClosed(() -> tops.cancelCompaction(tableName));
     expectClosed(() -> tops.listSplits(tableName));
+  }
 
+  @Test
+  public void testAmpleReadTablets() throws Exception {
+
+    try (AccumuloClient accumuloClient = Accumulo.newClient().from(getClientProps()).build()) {
+      accumuloClient.securityOperations().grantTablePermission(accumuloClient.whoami(),

Review comment:
       An alternative way to write this test would be to create a table w/ desired splits and then get the table ID.  

##########
File path: test/src/main/java/org/apache/accumulo/test/functional/AccumuloClientIT.java
##########
@@ -220,6 +238,120 @@ public void testClose() throws Exception {
     expectClosed(() -> tops.create("expectFail"));
     expectClosed(() -> tops.cancelCompaction(tableName));
     expectClosed(() -> tops.listSplits(tableName));
+  }
 
+  @Test
+  public void testAmpleReadTablets() throws Exception {

Review comment:
       Why put this test in AccumuloClientIT?

##########
File path: core/src/main/java/org/apache/accumulo/core/metadata/schema/AmpleImpl.java
##########
@@ -42,4 +43,12 @@ public TabletMetadata readTablet(KeyExtent extent, ColumnType... colsToFetch) {
       return Iterables.getOnlyElement(tablets);
     }
   }
+
+  @Override
+  public TabletsMetadata.Builder readTablets() {
+    this.builder = new TabletsMetadata.Builder();
+    builder._client = this.client;

Review comment:
       It would be cleaner to pass the client to the constructor if possible.

##########
File path: core/src/main/java/org/apache/accumulo/core/metadata/schema/AmpleImpl.java
##########
@@ -27,6 +27,7 @@
 
 public class AmpleImpl implements Ample {
   private final AccumuloClient client;
+  private TabletsMetadata.Builder builder;

Review comment:
       Why is this an instance variable?




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org