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 2021/02/23 15:01:40 UTC
[GitHub] [accumulo] milleruntime opened a new pull request #1945: Create readTablets method in Ample. Closes #1473
milleruntime opened a new pull request #1945:
URL: https://github.com/apache/accumulo/pull/1945
* Modify TabletsMetadata to impl readTablets in Ample
* Modify classes calling TabletsMetadata to pass the client object as
part of the builder init, instead of the final build method
* Add testAmpleReadTablets to MetadataIT
Co-authored-by: cradal <20...@users.noreply.github.com>
Updated version of #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 #1945: Create readTablets method in Ample. Closes #1473
Posted by GitBox <gi...@apache.org>.
milleruntime commented on a change in pull request #1945:
URL: https://github.com/apache/accumulo/pull/1945#discussion_r581302314
##########
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:
I'm not sure what this is guarding against... only one can be null and only one set? I feel like this would be less confusing as two separate checks.
----------------------------------------------------------------
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 #1945: Create readTablets method in Ample. Closes #1473
Posted by 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
[GitHub] [accumulo] milleruntime commented on a change in pull request #1945: Create readTablets method in Ample. Closes #1473
Posted by GitBox <gi...@apache.org>.
milleruntime commented on a change in pull request #1945:
URL: https://github.com/apache/accumulo/pull/1945#discussion_r581318794
##########
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:
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] milleruntime commented on a change in pull request #1945: Create readTablets method in Ample. Closes #1473
Posted by GitBox <gi...@apache.org>.
milleruntime commented on a change in pull request #1945:
URL: https://github.com/apache/accumulo/pull/1945#discussion_r581331110
##########
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:
For description: "Only the table or the level can be set" ? I am not sure that makes sense in the context of the builder.
----------------------------------------------------------------
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] EdColeman commented on a change in pull request #1945: Create readTablets method in Ample. Closes #1473
Posted by GitBox <gi...@apache.org>.
EdColeman commented on a change in pull request #1945:
URL: https://github.com/apache/accumulo/pull/1945#discussion_r581495555
##########
File path: core/src/main/java/org/apache/accumulo/core/clientImpl/bulk/ConcurrentKeyExtentCache.java
##########
@@ -83,8 +83,8 @@ protected void updateCache(KeyExtent e) {
@VisibleForTesting
protected Stream<KeyExtent> lookupExtents(Text row) {
- return TabletsMetadata.builder().forTable(tableId).overlapping(row, null).checkConsistency()
- .fetch(PREV_ROW).build(ctx).stream().limit(100).map(TabletMetadata::getExtent);
+ return TabletsMetadata.builder(ctx).forTable(tableId).overlapping(row, null).checkConsistency()
Review comment:
Would it improve readability if one of context or ctx was used consistently? The first two files reviewed used context, some of the rest seem to be using ctx - don't have a strong preference for either - context takes less mental translation, but ctx is almost as clear, and shorter.
----------------------------------------------------------------
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 #1945: Create readTablets method in Ample. Closes #1473
Posted by GitBox <gi...@apache.org>.
milleruntime commented on a change in pull request #1945:
URL: https://github.com/apache/accumulo/pull/1945#discussion_r581357299
##########
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:
Also looks like `forTablet()` as well. I will include it in your suggestion.
----------------------------------------------------------------
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 merged pull request #1945: Create readTablets method in Ample. Closes #1473
Posted by GitBox <gi...@apache.org>.
milleruntime merged pull request #1945:
URL: https://github.com/apache/accumulo/pull/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 #1945: Create readTablets method in Ample. Closes #1473
Posted by GitBox <gi...@apache.org>.
milleruntime commented on a change in pull request #1945:
URL: https://github.com/apache/accumulo/pull/1945#discussion_r581969475
##########
File path: core/src/main/java/org/apache/accumulo/core/clientImpl/bulk/ConcurrentKeyExtentCache.java
##########
@@ -83,8 +83,8 @@ protected void updateCache(KeyExtent e) {
@VisibleForTesting
protected Stream<KeyExtent> lookupExtents(Text row) {
- return TabletsMetadata.builder().forTable(tableId).overlapping(row, null).checkConsistency()
- .fetch(PREV_ROW).build(ctx).stream().limit(100).map(TabletMetadata::getExtent);
+ return TabletsMetadata.builder(ctx).forTable(tableId).overlapping(row, null).checkConsistency()
Review comment:
Possibly but I think that is unrelated to this change.
----------------------------------------------------------------
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 #1945: Create readTablets method in Ample. Closes #1473
Posted by GitBox <gi...@apache.org>.
milleruntime commented on a change in pull request #1945:
URL: https://github.com/apache/accumulo/pull/1945#discussion_r581364030
##########
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:
I also just attempted to write a test for this error and it would be difficult since the builder interfaces protect against this condition.
----------------------------------------------------------------
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 #1945: Create readTablets method in Ample. Closes #1473
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on a change in pull request #1945:
URL: https://github.com/apache/accumulo/pull/1945#discussion_r581354345
##########
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:
Maybe "scanTable() cannot be used in conjunction with forLevel() or forTable()". I didn't look too deep into why these can't be used together, but that seems to be the restriction that is being enforced.
----------------------------------------------------------------
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