milleruntime commented on a change in pull request #1651:
URL: https://github.com/apache/accumulo/pull/1651#discussion_r517649007
##########
File path:
server/base/src/test/java/org/apache/accumulo/server/master/state/RootTabletStateStoreTest.java
##########
@@ -54,6 +55,11 @@ public TabletMetadata readTablet(KeyExtent extent,
ColumnType... colsToFetch) {
return RootTabletMetadata.fromJson(json).convertToTabletMetadata();
}
+ @Override
+ public AmpleImpl.Builder readTablets() {
+ return null;
+ }
Review comment:
This should throw an exception so it is more clear that its not being
used.
##########
File path:
core/src/main/java/org/apache/accumulo/core/metadata/schema/AmpleImpl.java
##########
@@ -34,12 +70,212 @@ public AmpleImpl(AccumuloClient client) {
@Override
public TabletMetadata readTablet(KeyExtent extent, ColumnType...
colsToFetch) {
- Options builder = TabletsMetadata.builder().forTablet(extent);
+ TabletsMetadata.Options builder =
TabletsMetadata.builder().forTablet(extent);
if (colsToFetch.length > 0)
builder.fetch(colsToFetch);
try (TabletsMetadata tablets = builder.build(client)) {
return Iterables.getOnlyElement(tablets);
}
}
+
+ @Override
+ public AmpleImpl.Builder readTablets() {
+ Builder builder = new Builder(client);
+ return builder;
+ }
+
+ public static class Builder implements Iterable<TabletMetadata> {
Review comment:
I think this class should stay where it is now but just be made public.
It is difficult to review what you changed when you moved it here. It looks
like you made the correct changes with the `Builder(client)` constructor and
`build()` method but it is difficult to know for sure.
##########
File path:
core/src/main/java/org/apache/accumulo/core/metadata/schema/Ample.java
##########
@@ -119,6 +119,15 @@ public static DataLevel of(TableId tableId) {
*/
TabletMetadata readTablet(KeyExtent extent, 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.
+ */
+
+ AmpleImpl.Builder readTablets();
Review comment:
I think this method should return `TabletsMetadata.TableOptions`. That
will limit the next method calls to what is needed for the fluent entry point
and help prevent coding errors.
We want to select the table first like this:
```
ample.readTablets().forTable(tableId).fetch(LOCATION, PREV_ROW).build();
```
And not be able to skip the required parameters like this:
```
ample.readTablets().build(); //coding error
```
##########
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:
I don't think AccumuloClientIT is the appropriate place for testing this
method, this test is mainly used to test the AccumuloClient. I think somewhere
like MetadataIT would be a better place for verifying the metadata.
----------------------------------------------------------------
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:
[email protected]
