[GitHub] [accumulo] dlmarion commented on a diff in pull request #3332: WIP - Add an optional fenced range to Tablet files

["dlmarion (via GitHub)" <gi...@apache.org>]

dlmarion commented on code in PR #3332:
URL: https://github.com/apache/accumulo/pull/3332#discussion_r1175257497


##########
core/src/main/java/org/apache/accumulo/core/file/rfile/RFile.java:
##########
@@ -1553,4 +1556,117 @@ public void setCacheProvider(CacheProvider cacheProvider) {
       reader.setCacheProvider(cacheProvider);
     }
   }
+
+  interface IndexedFileSKVIterator extends FileSKVIterator {

Review Comment:
   I wonder if the word `Index` might create confusion here considering the RFile has an index. The name of this interface is meant to convey that this is bound to some range? What about `BoundedFileSKVIterator`?



##########
core/src/main/java/org/apache/accumulo/core/file/rfile/RFile.java:
##########
@@ -1553,4 +1556,117 @@ public void setCacheProvider(CacheProvider cacheProvider) {
       reader.setCacheProvider(cacheProvider);
     }
   }
+
+  interface IndexedFileSKVIterator extends FileSKVIterator {
+    FileSKVIterator getIndex() throws IOException;
+  }
+
+  static class FencedReader implements IndexedFileSKVIterator {
+
+    private final Reader reader;
+    private final Range fence;
+
+    public FencedReader(Reader reader, Range seekFence) {
+      this.reader = Objects.requireNonNull(reader);
+      this.fence = seekFence;
+    }
+
+    @Override
+    public void seek(Range range, Collection<ByteSequence> columnFamilies, boolean inclusive)
+        throws IOException {
+      reader.clear();
+
+      if (fence != null) {
+        range = fence.clip(range, true);
+        if (range == null) {
+          return;
+        }
+      }
+
+      reader.seek(range, columnFamilies, inclusive);

Review Comment:
   It appears that this just uses the Rows of the start/stop Keys and not the entire Key object. Could you use the RowRange object that Dom is working on here?



##########
core/src/main/java/org/apache/accumulo/core/file/FileOperations.java:
##########
@@ -411,13 +419,26 @@ public class ReaderBuilder extends FileHelper implements ReaderTableConfiguratio
     private CacheProvider cacheProvider;
     private Cache<String,Long> fileLenCache;
     private boolean seekToBeginning = false;
+    private Range fence;
 
     public ReaderTableConfiguration forFile(String filename, FileSystem fs, Configuration fsConf,
         CryptoService cs) {
       filename(filename).fs(fs).fsConf(fsConf).cryptoService(cs);
       return this;
     }
 
+    public ReaderTableConfiguration forFile(TabletFile file, FileSystem fs, Configuration fsConf,
+        CryptoService cs) {
+      return forFile(file.getPathStr(), file.getFence(), fs, fsConf, cs);
+    }
+
+    public ReaderTableConfiguration forFile(String filename, Range fence, FileSystem fs,

Review Comment:
   Would creating a `withRange` method reduce the number of overall changes? Calling code could be:
   ```
   forFile(file, fs, conf, cs).withRange(range)
   ```



##########
core/src/main/java/org/apache/accumulo/core/file/rfile/RFileOperations.java:
##########
@@ -53,13 +56,16 @@ public class RFileOperations extends FileOperations {
 
   private static final Collection<ByteSequence> EMPTY_CF_SET = Collections.emptySet();
 
-  private static RFile.Reader getReader(FileOptions options) throws IOException {
+  private static IndexedFileSKVIterator getReader(FileOptions options) throws IOException {
     CachableBuilder cb = new CachableBuilder()
         .fsPath(options.getFileSystem(), new Path(options.getFilename()), options.dropCacheBehind)
         .conf(options.getConfiguration()).fileLen(options.getFileLenCache())
         .cacheProvider(options.cacheProvider).readLimiter(options.getRateLimiter())
         .cryptoService(options.getCryptoService());
-    return new RFile.Reader(cb);
+    final RFile.Reader reader = new RFile.Reader(cb);
+    return Optional.ofNullable(options.getFence())

Review Comment:
   ```suggestion
       return options.getFence() == null ? reader : new FencedReader(reader, options.getFence());
   ```
   
   Functionally equivalent, but doesn't create objects.



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