[GitHub] [accumulo] ctubbsii commented on a change in pull request #2189: Make Ample only return exact matches for KeyExtents

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

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



##########
File path: core/src/main/java/org/apache/accumulo/core/metadata/schema/AmpleImpl.java
##########
@@ -44,7 +45,14 @@ public TabletMetadata readTablet(KeyExtent extent, ReadConsistency readConsisten
     builder.readConsistency(readConsistency);
 
     try (TabletsMetadata tablets = builder.build()) {
-      return Iterables.getOnlyElement(tablets);
+      TabletMetadata tmd = Iterables.getOnlyElement(tablets);
+      Text tmpExtent = extent.prevEndRow();
+      Text tmpTmd = tmd.getPrevEndRow();
+      if (tmpExtent == tmpTmd || tmpExtent.equals(tmpTmd)) {
+        return tmd;
+      } else {
+        return null;
+      }

Review comment:
       This is shorter, and much easier to read. It also handles the cases where the prevEndRow is null.
   
   ```suggestion
         return Objects.equals(extent.prevEndRow(), tmd.getPrevEndRow()) ? tmd : null;
   ```

##########
File path: core/src/main/java/org/apache/accumulo/core/metadata/schema/TabletsMetadata.java
##########
@@ -290,7 +290,8 @@ public TableRangeOptions forTable(TableId tableId) {
     @Override
     public Options forTablet(KeyExtent extent) {
       forTable(extent.tableId());
-      this.range = new Range(extent.toMetaRow());
+      // this.range = new Range(extent.toMetaRow());
+      this.range = new Range(extent.toMetaRange());

Review comment:
       `extent.toMetaRange()` already returns a `Range` type. It's not necessary to copy it again.
   
   ```suggestion
         this.range = extent.toMetaRange();
   ```
   
   This change to use the `toMetaRange` rather than `toMetaRow` seems risky. There are many places in our code where we tend to throw "null" into the prevEndRow when constructing a KeyExtent, just because we know we were using it only to carry the TableId and endRow, and we didn't necessarily care about the value of the prevEndRow. Since this change now makes us actually use the prevEndRow, we need to make sure that any callers aren't breaking things by using null for the prevEndRow when constructing the KeyExtent that is used here.
   
   I don't have a good grasp of the assumptions that code that calls this might make, and how it might be affected by this change. This probably warrants further 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.

To unsubscribe, e-mail: notifications-unsubscribe@accumulo.apache.org

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