You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@accumulo.apache.org by GitBox <gi...@apache.org> on 2020/07/07 12:16:00 UTC
[GitHub] [accumulo] cradal opened a new pull request #1651: WIP #1473 Move TabletsMetadata builder to Ample
cradal opened a new pull request #1651:
URL: https://github.com/apache/accumulo/pull/1651
----------------------------------------------------------------
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
[GitHub] [accumulo] milleruntime commented on a change in pull request #1651: #1473 Move TabletsMetadata builder to Ample
Posted by GitBox <gi...@apache.org>.
milleruntime commented on a change in pull request #1651:
URL: https://github.com/apache/accumulo/pull/1651#discussion_r527071161
##########
File path: core/src/main/java/org/apache/accumulo/core/metadata/schema/TabletsMetadata.java
##########
@@ -79,6 +79,15 @@
private boolean checkConsistency = false;
private boolean saveKeyValues;
private TableId tableId;
+ private AccumuloClient _client;
+
+ Builder(AccumuloClient client) {
+ this._client = client;
+ }
+
+ Builder() {
+
+ }
Review comment:
That is OK. I should only be 1 or 2 lines per class.
----------------------------------------------------------------
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
[GitHub] [accumulo] cradal commented on a change in pull request #1651: #1473 Move TabletsMetadata builder to Ample
Posted by GitBox <gi...@apache.org>.
cradal commented on a change in pull request #1651:
URL: https://github.com/apache/accumulo/pull/1651#discussion_r526200209
##########
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:
Added UnsupportedOperationException.
----------------------------------------------------------------
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
[GitHub] [accumulo] cradal commented on a change in pull request #1651: #1473 Move TabletsMetadata builder to Ample
Posted by GitBox <gi...@apache.org>.
cradal commented on a change in pull request #1651:
URL: https://github.com/apache/accumulo/pull/1651#discussion_r512166130
##########
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:
This is here to facilitate calling the method from a AccumuloClient.
----------------------------------------------------------------
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
[GitHub] [accumulo] cradal commented on pull request #1651: #1473 Move TabletsMetadata builder to Ample
Posted by GitBox <gi...@apache.org>.
cradal commented on pull request #1651:
URL: https://github.com/apache/accumulo/pull/1651#issuecomment-696794553
@ctubbsii Just a reminder that this PR is still available for review.
----------------------------------------------------------------
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
[GitHub] [accumulo] ctubbsii commented on a change in pull request #1651: #1473 Move TabletsMetadata builder to Ample
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on a change in pull request #1651:
URL: https://github.com/apache/accumulo/pull/1651#discussion_r492907530
##########
File path: core/src/main/java/org/apache/accumulo/core/metadata/schema/TabletsMetadata.java
##########
@@ -79,6 +79,7 @@
private boolean checkConsistency = false;
private boolean saveKeyValues;
private TableId tableId;
+ public AccumuloClient _client;
Review comment:
public non-final variables should be avoided.
##########
File path: core/src/main/java/org/apache/accumulo/core/metadata/schema/TabletsMetadata.java
##########
@@ -93,6 +94,20 @@ public TabletsMetadata build(AccumuloClient client) {
}
}
+ // No-parameter build method that allows the use of AmpleImpl's Accumulo client.
+ // Ample methods can assign AmpleImple's Accumulo client to _client of this Builder object.
+ public TabletsMetadata build() {
+ Preconditions.checkState(level == null ^ table == null);
Review comment:
This might be slightly more readable as:
```suggestion
Preconditions.checkState((level == null) != (table == null));
```
##########
File path: core/src/main/java/org/apache/accumulo/core/metadata/schema/TabletsMetadata.java
##########
@@ -371,6 +388,10 @@ private TabletsMetadata(Scanner scanner, Iterable<TabletMetadata> tmi) {
this.tablets = tmi;
}
+ public TabletsMetadata() {
+
+ }
+
Review comment:
This doesn't appear to be used. Not sure it's needed.
----------------------------------------------------------------
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
[GitHub] [accumulo] milleruntime commented on pull request #1651: #1473 Move TabletsMetadata builder to Ample
Posted by GitBox <gi...@apache.org>.
milleruntime commented on pull request #1651:
URL: https://github.com/apache/accumulo/pull/1651#issuecomment-721364684
Is this change complete? I would like to use this to read metadata in a new admin command.
----------------------------------------------------------------
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
[GitHub] [accumulo] milleruntime commented on pull request #1651: #1473 Move TabletsMetadata builder to Ample
Posted by GitBox <gi...@apache.org>.
milleruntime commented on pull request #1651:
URL: https://github.com/apache/accumulo/pull/1651#issuecomment-783650277
@cradal If you don't have any objections, I will go ahead and finish up the work on this PR.
----------------------------------------------------------------
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
[GitHub] [accumulo] cradal commented on a change in pull request #1651: #1473 Move TabletsMetadata builder to Ample
Posted by GitBox <gi...@apache.org>.
cradal commented on a change in pull request #1651:
URL: https://github.com/apache/accumulo/pull/1651#discussion_r526200407
##########
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:
Moved test to MetadataIT.
----------------------------------------------------------------
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
[GitHub] [accumulo] cradal commented on pull request #1651: WIP #1473 Move TabletsMetadata builder to Ample
Posted by GitBox <gi...@apache.org>.
cradal commented on pull request #1651:
URL: https://github.com/apache/accumulo/pull/1651#issuecomment-655021719
This code change makes the Builder class public instead of private. This was done to give readTablets() access to instantiate a Builder object, allowing use TabletsMetadata Builder methods and implementations.
----------------------------------------------------------------
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
[GitHub] [accumulo] milleruntime commented on a change in pull request #1651: #1473 Move TabletsMetadata builder to Ample
Posted by GitBox <gi...@apache.org>.
milleruntime commented on a change in pull request #1651:
URL: https://github.com/apache/accumulo/pull/1651#discussion_r561902312
##########
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:
Old modifications that can be dropped.
----------------------------------------------------------------
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
[GitHub] [accumulo] cradal commented on pull request #1651: #1473 Move TabletsMetadata builder to Ample
Posted by GitBox <gi...@apache.org>.
cradal commented on pull request #1651:
URL: https://github.com/apache/accumulo/pull/1651#issuecomment-661826380
@keith-turner This is ready to be reviewed. I went with making Builder a protected class vice package-private. I think this provides a good balance between encapsulating Builder and making it available to the test classes.
----------------------------------------------------------------
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
[GitHub] [accumulo] cradal commented on a change in pull request #1651: #1473 Move TabletsMetadata builder to Ample
Posted by GitBox <gi...@apache.org>.
cradal commented on a change in pull request #1651:
URL: https://github.com/apache/accumulo/pull/1651#discussion_r526262529
##########
File path: core/src/main/java/org/apache/accumulo/core/metadata/schema/TabletsMetadata.java
##########
@@ -79,6 +79,15 @@
private boolean checkConsistency = false;
private boolean saveKeyValues;
private TableId tableId;
+ private AccumuloClient _client;
+
+ Builder(AccumuloClient client) {
+ this._client = client;
+ }
+
+ Builder() {
+
+ }
Review comment:
I might be able to refactor those classes to use the new Builder(Accumulo client) constructor.
----------------------------------------------------------------
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
[GitHub] [accumulo] cradal commented on a change in pull request #1651: #1473 Move TabletsMetadata builder to Ample
Posted by GitBox <gi...@apache.org>.
cradal commented on a change in pull request #1651:
URL: https://github.com/apache/accumulo/pull/1651#discussion_r512165079
##########
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:
Latest push removes this var.
----------------------------------------------------------------
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
[GitHub] [accumulo] cradal commented on a change in pull request #1651: #1473 Move TabletsMetadata builder to Ample
Posted by GitBox <gi...@apache.org>.
cradal commented on a change in pull request #1651:
URL: https://github.com/apache/accumulo/pull/1651#discussion_r529731488
##########
File path: core/src/main/java/org/apache/accumulo/core/metadata/schema/TabletsMetadata.java
##########
@@ -79,6 +79,15 @@
private boolean checkConsistency = false;
private boolean saveKeyValues;
private TableId tableId;
+ private AccumuloClient _client;
+
+ Builder(AccumuloClient client) {
+ this._client = client;
+ }
+
+ Builder() {
+
+ }
Review comment:
In the latest push: Refactored the other classes to use the new constructor. Cleaned up/streamlined unit test in MetadataIT.
----------------------------------------------------------------
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
[GitHub] [accumulo] cradal commented on pull request #1651: #1473 Move TabletsMetadata builder to Ample
Posted by GitBox <gi...@apache.org>.
cradal commented on pull request #1651:
URL: https://github.com/apache/accumulo/pull/1651#issuecomment-783824690
@milleruntime No objections.
----------------------------------------------------------------
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
[GitHub] [accumulo] milleruntime commented on pull request #1651: #1473 Move TabletsMetadata builder to Ample
Posted by GitBox <gi...@apache.org>.
milleruntime commented on pull request #1651:
URL: https://github.com/apache/accumulo/pull/1651#issuecomment-763625181
@cradal Do you have plans on finishing this PR? If not, I can get take it over if you'd like. I recently used this code and have at least one place it could be used in #1821
----------------------------------------------------------------
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
[GitHub] [accumulo] milleruntime commented on a change in pull request #1651: #1473 Move TabletsMetadata builder to Ample
Posted by GitBox <gi...@apache.org>.
milleruntime commented on a change in pull request #1651:
URL: https://github.com/apache/accumulo/pull/1651#discussion_r516948397
##########
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);
Review comment:
A very simple alternative to this change, would be to just have a shared method return `TabletsMetadata` (before calling getOnlyElement). Is there a reason just returning `TabletsMetadata` for readTablets() wouldn't suffice?
The method on the interface could look like this:
`public TabletsMetadata readTablets(KeyExtent extent, ColumnType... colsToFetch)`
----------------------------------------------------------------
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
[GitHub] [accumulo] milleruntime commented on a change in pull request #1651: #1473 Move TabletsMetadata builder to Ample
Posted by GitBox <gi...@apache.org>.
milleruntime commented on a change in pull request #1651:
URL: https://github.com/apache/accumulo/pull/1651#discussion_r526250613
##########
File path: core/src/main/java/org/apache/accumulo/core/metadata/schema/TabletsMetadata.java
##########
@@ -79,6 +79,15 @@
private boolean checkConsistency = false;
private boolean saveKeyValues;
private TableId tableId;
+ private AccumuloClient _client;
+
+ Builder(AccumuloClient client) {
+ this._client = client;
+ }
+
+ Builder() {
+
+ }
Review comment:
Should only have one constructor and one build method to prevent coding errors. Since you have access to the AccumuloClient object at the time you are calling the method, I think it would be best to go with the `Builder(AccumuloClient client)` constructor and then end it with the `build()` method. This will only pass in the client once and prevent it from being null when calling `build()`.
----------------------------------------------------------------
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
[GitHub] [accumulo] ctubbsii commented on pull request #1651: #1473 Move TabletsMetadata builder to Ample
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on pull request #1651:
URL: https://github.com/apache/accumulo/pull/1651#issuecomment-763981604
I attempted to resolve the merge conflicts to help out in getting this PR back into a healthy state. I will leave the rest to @cradal and/or @milleruntime
----------------------------------------------------------------
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
[GitHub] [accumulo] milleruntime closed pull request #1651: #1473 Move TabletsMetadata builder to Ample
Posted by GitBox <gi...@apache.org>.
milleruntime closed pull request #1651:
URL: https://github.com/apache/accumulo/pull/1651
----------------------------------------------------------------
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
[GitHub] [accumulo] cradal commented on a change in pull request #1651: #1473 Move TabletsMetadata builder to Ample
Posted by GitBox <gi...@apache.org>.
cradal commented on a change in pull request #1651:
URL: https://github.com/apache/accumulo/pull/1651#discussion_r526240853
##########
File path: test/src/main/java/org/apache/accumulo/test/functional/MetadataIT.java
##########
@@ -156,4 +171,122 @@ public void batchScanTest() throws Exception {
}
}
}
+
+ @Test
+ public void testAmpleReadTablets() throws Exception {
+
+ try (AccumuloClient accumuloClient = Accumulo.newClient().from(getClientProps()).build()) {
+ accumuloClient.securityOperations().grantTablePermission(accumuloClient.whoami(),
+ MetadataTable.NAME, TablePermission.WRITE);
+ BatchWriter bw =
+ accumuloClient.createBatchWriter(MetadataTable.NAME, new BatchWriterConfig());
+ ClientContext cc = (ClientContext) accumuloClient;
+ // Create a fake METADATA table with these splits
Review comment:
That idea is more economical. I guess an effective test does not always need a full-blown client. I will update the test.
----------------------------------------------------------------
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
[GitHub] [accumulo] cradal commented on pull request #1651: #1473 Move TabletsMetadata builder to Ample
Posted by GitBox <gi...@apache.org>.
cradal commented on pull request #1651:
URL: https://github.com/apache/accumulo/pull/1651#issuecomment-696794553
@ctubbsii Just a reminder that this PR is still available for review.
----------------------------------------------------------------
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
[GitHub] [accumulo] milleruntime commented on a change in pull request #1651: #1473 Move TabletsMetadata builder to Ample
Posted by GitBox <gi...@apache.org>.
milleruntime commented on a change in pull request #1651:
URL: https://github.com/apache/accumulo/pull/1651#discussion_r561902312
##########
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:
Old modifications that can be dropped.
----------------------------------------------------------------
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
[GitHub] [accumulo] cradal commented on a change in pull request #1651: WIP #1473 Move TabletsMetadata builder to Ample
Posted by GitBox <gi...@apache.org>.
cradal commented on a change in pull request #1651:
URL: https://github.com/apache/accumulo/pull/1651#discussion_r452842622
##########
File path: core/src/main/java/org/apache/accumulo/core/metadata/schema/TabletsMetadata.java
##########
@@ -67,7 +67,7 @@
*/
public class TabletsMetadata implements Iterable<TabletMetadata>, AutoCloseable {
- private static class Builder implements TableRangeOptions, TableOptions, RangeOptions, Options {
+ public static class Builder implements TableRangeOptions, TableOptions, RangeOptions, Options {
Review comment:
@ivakegg I think this is the way to go. One tumbling block is crafting a unit test that is outside the ...metadata.schema package. A test created outside that package won't have access to the Builder methods. For the moment, I will push a version with a public Builder class that is tested using a mock table.
----------------------------------------------------------------
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
[GitHub] [accumulo] cradal commented on a change in pull request #1651: #1473 Move TabletsMetadata builder to Ample
Posted by GitBox <gi...@apache.org>.
cradal commented on a change in pull request #1651:
URL: https://github.com/apache/accumulo/pull/1651#discussion_r526199699
##########
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:
Incorporated suggestions on latest push.
----------------------------------------------------------------
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
[GitHub] [accumulo] milleruntime commented on a change in pull request #1651: #1473 Move TabletsMetadata builder to Ample
Posted by GitBox <gi...@apache.org>.
milleruntime commented on a change in pull request #1651:
URL: https://github.com/apache/accumulo/pull/1651#discussion_r526213844
##########
File path: test/src/main/java/org/apache/accumulo/test/functional/MetadataIT.java
##########
@@ -156,4 +171,122 @@ public void batchScanTest() throws Exception {
}
}
}
+
+ @Test
+ public void testAmpleReadTablets() throws Exception {
+
+ try (AccumuloClient accumuloClient = Accumulo.newClient().from(getClientProps()).build()) {
+ accumuloClient.securityOperations().grantTablePermission(accumuloClient.whoami(),
+ MetadataTable.NAME, TablePermission.WRITE);
+ BatchWriter bw =
+ accumuloClient.createBatchWriter(MetadataTable.NAME, new BatchWriterConfig());
+ ClientContext cc = (ClientContext) accumuloClient;
+ // Create a fake METADATA table with these splits
Review comment:
Why not just use the API to create a table with those splits and let Accumulo populate 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:
users@infra.apache.org
[GitHub] [accumulo] cradal commented on a change in pull request #1651: #1473 Move TabletsMetadata builder to Ample
Posted by GitBox <gi...@apache.org>.
cradal commented on a change in pull request #1651:
URL: https://github.com/apache/accumulo/pull/1651#discussion_r495046947
##########
File path: core/src/main/java/org/apache/accumulo/core/metadata/schema/TabletsMetadata.java
##########
@@ -79,6 +79,7 @@
private boolean checkConsistency = false;
private boolean saveKeyValues;
private TableId tableId;
+ public AccumuloClient _client;
Review comment:
This was done as part of an overall attempt take the builder interfaces in TabletsMetadata and move them to Ample, while leaving the implementation of the interfaces in TabletsMetadata.The readTablets() method in AmplImpl generates a TabletsMetadata builder object and assigns the AmpleImpl client to the public client in that builder object. This allows readTablets() to be called from a ClientContext. However, at this point, I think I should rethink the whole design because it breaks too many coding conventions (case in point, above).
##########
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:
This was done as part of an overall attempt take the builder interfaces in TabletsMetadata and move them to Ample, while leaving the implementation of the interfaces in TabletsMetadata.The readTablets() method in AmplImpl generates a TabletsMetadata builder object and assigns the AmpleImpl client to the public client in that builder object. This allows readTablets() to be called from a ClientContext. However, at this point, I think I should rethink the whole design because it breaks too many coding conventions (case in point, above).
----------------------------------------------------------------
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
[GitHub] [accumulo] cradal commented on pull request #1651: #1473 Move TabletsMetadata builder to Ample
Posted by GitBox <gi...@apache.org>.
cradal commented on pull request #1651:
URL: https://github.com/apache/accumulo/pull/1651#issuecomment-716727346
Most recent push encapsulates Builder member vars and moves Builder class over to AmpleImpl. Refactored ZooKeeperInstance to use new AmpleImpl method.
----------------------------------------------------------------
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
[GitHub] [accumulo] cradal commented on pull request #1651: #1473 Move TabletsMetadata builder to Ample
Posted by GitBox <gi...@apache.org>.
cradal commented on pull request #1651:
URL: https://github.com/apache/accumulo/pull/1651#issuecomment-763920242
I would like to finish it up. I just wasn't sure if the changes I last made
were okay or not.
On Wed, Jan 20, 2021, 9:02 AM Mike Miller <no...@github.com> wrote:
> @cradal <https://github.com/cradal> Do you have plans on finishing this
> PR? If not, I can get take it over if you'd like. I recently used this code
> and have at least one place it could be used in #1821
> <https://github.com/apache/accumulo/pull/1821>
>
> —
> You are receiving this because you were mentioned.
> Reply to this email directly, view it on GitHub
> <https://github.com/apache/accumulo/pull/1651#issuecomment-763625181>, or
> unsubscribe
> <https://github.com/notifications/unsubscribe-auth/AE242AMLIHC6N7K6OFLBE7LS23O7RANCNFSM4OSYFTZQ>
> .
>
----------------------------------------------------------------
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
[GitHub] [accumulo] milleruntime commented on pull request #1651: WIP #1473 Move TabletsMetadata builder to Ample
Posted by GitBox <gi...@apache.org>.
milleruntime commented on pull request #1651:
URL: https://github.com/apache/accumulo/pull/1651#issuecomment-658222896
@cradal I noticed you opened this PR in addition to another one you still have open: https://github.com/apache/accumulo/pull/1563. If this PR supersedes #1563 then please close that one.
----------------------------------------------------------------
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
[GitHub] [accumulo] cradal commented on a change in pull request #1651: #1473 Move TabletsMetadata builder to Ample
Posted by GitBox <gi...@apache.org>.
cradal commented on a change in pull request #1651:
URL: https://github.com/apache/accumulo/pull/1651#discussion_r526200967
##########
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:
Incorporated suggestions on latest push.
----------------------------------------------------------------
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
[GitHub] [accumulo] ivakegg commented on a change in pull request #1651: WIP #1473 Move TabletsMetadata builder to Ample
Posted by GitBox <gi...@apache.org>.
ivakegg commented on a change in pull request #1651:
URL: https://github.com/apache/accumulo/pull/1651#discussion_r450941660
##########
File path: core/src/main/java/org/apache/accumulo/core/metadata/schema/TabletsMetadata.java
##########
@@ -67,7 +67,7 @@
*/
public class TabletsMetadata implements Iterable<TabletMetadata>, AutoCloseable {
- private static class Builder implements TableRangeOptions, TableOptions, RangeOptions, Options {
+ public static class Builder implements TableRangeOptions, TableOptions, RangeOptions, Options {
Review comment:
How about package private?
----------------------------------------------------------------
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
[GitHub] [accumulo] milleruntime commented on pull request #1651: #1473 Move TabletsMetadata builder to Ample
Posted by GitBox <gi...@apache.org>.
milleruntime commented on pull request #1651:
URL: https://github.com/apache/accumulo/pull/1651#issuecomment-784265219
Replaced by #1945
----------------------------------------------------------------
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
[GitHub] [accumulo] milleruntime commented on a change in pull request #1651: #1473 Move TabletsMetadata builder to Ample
Posted by GitBox <gi...@apache.org>.
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:
users@infra.apache.org
[GitHub] [accumulo] milleruntime commented on a change in pull request #1651: #1473 Move TabletsMetadata builder to Ample
Posted by GitBox <gi...@apache.org>.
milleruntime commented on a change in pull request #1651:
URL: https://github.com/apache/accumulo/pull/1651#discussion_r516945149
##########
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:
This exposes the Impl class on the interface. This breaks the contract of the interface.
----------------------------------------------------------------
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
[GitHub] [accumulo] ctubbsii commented on a change in pull request #1651: #1473 Move TabletsMetadata builder to Ample
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on a change in pull request #1651:
URL: https://github.com/apache/accumulo/pull/1651#discussion_r492907530
##########
File path: core/src/main/java/org/apache/accumulo/core/metadata/schema/TabletsMetadata.java
##########
@@ -79,6 +79,7 @@
private boolean checkConsistency = false;
private boolean saveKeyValues;
private TableId tableId;
+ public AccumuloClient _client;
Review comment:
public non-final variables should be avoided.
##########
File path: core/src/main/java/org/apache/accumulo/core/metadata/schema/TabletsMetadata.java
##########
@@ -93,6 +94,20 @@ public TabletsMetadata build(AccumuloClient client) {
}
}
+ // No-parameter build method that allows the use of AmpleImpl's Accumulo client.
+ // Ample methods can assign AmpleImple's Accumulo client to _client of this Builder object.
+ public TabletsMetadata build() {
+ Preconditions.checkState(level == null ^ table == null);
Review comment:
This might be slightly more readable as:
```suggestion
Preconditions.checkState((level == null) != (table == null));
```
##########
File path: core/src/main/java/org/apache/accumulo/core/metadata/schema/TabletsMetadata.java
##########
@@ -371,6 +388,10 @@ private TabletsMetadata(Scanner scanner, Iterable<TabletMetadata> tmi) {
this.tablets = tmi;
}
+ public TabletsMetadata() {
+
+ }
+
Review comment:
This doesn't appear to be used. Not sure it's needed.
----------------------------------------------------------------
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
[GitHub] [accumulo] cradal commented on pull request #1651: #1473 Move TabletsMetadata builder to Ample
Posted by GitBox <gi...@apache.org>.
cradal commented on pull request #1651:
URL: https://github.com/apache/accumulo/pull/1651#issuecomment-763920242
I would like to finish it up. I just wasn't sure if the changes I last made
were okay or not.
On Wed, Jan 20, 2021, 9:02 AM Mike Miller <no...@github.com> wrote:
> @cradal <https://github.com/cradal> Do you have plans on finishing this
> PR? If not, I can get take it over if you'd like. I recently used this code
> and have at least one place it could be used in #1821
> <https://github.com/apache/accumulo/pull/1821>
>
> —
> You are receiving this because you were mentioned.
> Reply to this email directly, view it on GitHub
> <https://github.com/apache/accumulo/pull/1651#issuecomment-763625181>, or
> unsubscribe
> <https://github.com/notifications/unsubscribe-auth/AE242AMLIHC6N7K6OFLBE7LS23O7RANCNFSM4OSYFTZQ>
> .
>
----------------------------------------------------------------
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
[GitHub] [accumulo] cradal commented on pull request #1651: WIP #1473 Move TabletsMetadata builder to Ample
Posted by GitBox <gi...@apache.org>.
cradal commented on pull request #1651:
URL: https://github.com/apache/accumulo/pull/1651#issuecomment-656680603
@keith-turner @ivakegg This version of the solution changes the modifier of TabletsMetadata Builder from private to public. This allows readTablets() in AmpleImpl to create a Builder object. I created a test using a mock table, which runs successfully. I think that package-private is a better choice for Builder, just need to figure out a way to test things outside of TabletsMetadata's package(...schema.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:
users@infra.apache.org
[GitHub] [accumulo] cradal commented on a change in pull request #1651: #1473 Move TabletsMetadata builder to Ample
Posted by GitBox <gi...@apache.org>.
cradal commented on a change in pull request #1651:
URL: https://github.com/apache/accumulo/pull/1651#discussion_r526260981
##########
File path: core/src/main/java/org/apache/accumulo/core/metadata/schema/TabletsMetadata.java
##########
@@ -79,6 +79,15 @@
private boolean checkConsistency = false;
private boolean saveKeyValues;
private TableId tableId;
+ private AccumuloClient _client;
+
+ Builder(AccumuloClient client) {
+ this._client = client;
+ }
+
+ Builder() {
+
+ }
Review comment:
One issue with this is that there are ~15 classes that already use the pre-existing no-parameter constructor. Perhaps I should find a different way to pass the AmpleImpl Accumulo client into the TabletsMetadata builder object.
----------------------------------------------------------------
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
[GitHub] [accumulo] ctubbsii commented on pull request #1651: #1473 Move TabletsMetadata builder to Ample
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on pull request #1651:
URL: https://github.com/apache/accumulo/pull/1651#issuecomment-733152771
@cradal There are several comments from older iterations of this, that are possibly no longer relevant. For those conversations, if you have already incorporated the suggestions, you can mark them as "Resolved", so that way the UI doesn't show them as prominently. That will make it easier to track current, ongoing discussions related to the latest iteration of this PR.
----------------------------------------------------------------
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
[GitHub] [accumulo] cradal edited a comment on pull request #1651: #1473 Move TabletsMetadata builder to Ample
Posted by GitBox <gi...@apache.org>.
cradal edited a comment on pull request #1651:
URL: https://github.com/apache/accumulo/pull/1651#issuecomment-661826380
@keith-turner This is ready to be reviewed. I went with making Builder a protected class vice package-private. This solves a conflict with RootTabletStoreTest, which implements Ample, but is in a different package than TabletsMetadata.
----------------------------------------------------------------
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
[GitHub] [accumulo] ctubbsii commented on pull request #1651: #1473 Move TabletsMetadata builder to Ample
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on pull request #1651:
URL: https://github.com/apache/accumulo/pull/1651#issuecomment-763981604
I attempted to resolve the merge conflicts to help out in getting this PR back into a healthy state. I will leave the rest to @cradal and/or @milleruntime
----------------------------------------------------------------
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
[GitHub] [accumulo] milleruntime commented on a change in pull request #1651: #1473 Move TabletsMetadata builder to Ample
Posted by GitBox <gi...@apache.org>.
milleruntime commented on a change in pull request #1651:
URL: https://github.com/apache/accumulo/pull/1651#discussion_r517647395
##########
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);
Review comment:
Nevermind, it doesn't look like this would work with the way that we use TabletsMetadata.
----------------------------------------------------------------
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
[GitHub] [accumulo] keith-turner commented on a change in pull request #1651: #1473 Move TabletsMetadata builder to Ample
Posted by 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