[GitHub] [accumulo] ctubbsii commented on a change in pull request #1945: Create readTablets method in Ample. Closes #1473

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

ctubbsii commented on a change in pull request #1945:
URL: https://github.com/apache/accumulo/pull/1945#discussion_r581172513



##########
File path: core/src/main/java/org/apache/accumulo/core/metadata/schema/AmpleImpl.java
##########
@@ -35,14 +35,21 @@ public AmpleImpl(AccumuloClient client) {
   @Override
   public TabletMetadata readTablet(KeyExtent extent, ReadConsistency readConsistency,
       ColumnType... colsToFetch) {
-    Options builder = TabletsMetadata.builder().forTablet(extent);
+    Options builder = TabletsMetadata.builder(client).forTablet(extent);
     if (colsToFetch.length > 0)
       builder.fetch(colsToFetch);
 
     builder.readConsistency(readConsistency);
 
-    try (TabletsMetadata tablets = builder.build(client)) {
+    try (TabletsMetadata tablets = builder.build()) {
       return Iterables.getOnlyElement(tablets);
     }
   }
+
+  @Override
+  public TabletsMetadata.TableOptions readTablets() {
+    TabletsMetadata.TableOptions builder = TabletsMetadata.builder(this.client);
+    return builder;
+  }

Review comment:
       ```suggestion
     public TabletsMetadata.TableOptions readTablets() {
       return TabletsMetadata.builder(this.client);
     }
   ```

##########
File path: test/src/main/java/org/apache/accumulo/test/functional/AccumuloClientIT.java
##########
@@ -46,11 +46,15 @@
 import org.apache.accumulo.harness.AccumuloClusterHarness;
 import org.junit.After;
 import org.junit.Test;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
 
 import com.google.common.collect.Iterables;
 
 public class AccumuloClientIT extends AccumuloClusterHarness {
 
+  Logger log = LoggerFactory.getLogger(AccumuloClientIT.class);

Review comment:
       This seems to be a new logger, but I don't see any new log messages in this PR. Is this unused?

##########
File path: core/src/main/java/org/apache/accumulo/core/metadata/schema/TabletsMetadata.java
##########
@@ -85,15 +85,20 @@
     private boolean saveKeyValues;
     private TableId tableId;
     private ReadConsistency readConsistency = ReadConsistency.IMMEDIATE;
+    private AccumuloClient _client;
+
+    Builder(AccumuloClient client) {
+      this._client = client;
+    }
 
     @Override
-    public TabletsMetadata build(AccumuloClient client) {
-      Preconditions.checkState(level == null ^ table == null);
+    public TabletsMetadata build() {
+      Preconditions.checkState((level == null) != (table == null));

Review comment:
       This precondition should have an accompanying description.

##########
File path: core/src/main/java/org/apache/accumulo/core/metadata/schema/Ample.java
##########
@@ -153,6 +153,15 @@ default TabletMetadata readTablet(KeyExtent extent, ColumnType... colsToFetch) {
   TabletMetadata readTablet(KeyExtent extent, ReadConsistency readConsistency,
       ColumnType... colsToFetch);
 
+  /**
+   * Entry point for reading multiple tablets' metadata. Generates a TabletsMetadata builder object
+   * and assigns the AmpleImpl client to that builder object. This allows readTablets() to be called
+   * from a ClientContext. Associated methods of the TabletsMetadata Builder class are used to
+   * generate the metadata.
+   */
+
+  TabletsMetadata.TableOptions readTablets();

Review comment:
       I wish we could enforce that there's no blank line between a method and its linked javadoc in the formatter, but I haven't found such a config.
   ```suggestion
      */
     TabletsMetadata.TableOptions readTablets();
   ```




----------------------------------------------------------------
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