accumulo-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From joshelser <...@git.apache.org>
Subject [GitHub] accumulo pull request #260: ACCUMULO-4643 initial implementation
Date Tue, 30 May 2017 16:09:10 GMT
Github user joshelser commented on a diff in the pull request:

    https://github.com/apache/accumulo/pull/260#discussion_r119144555
  
    --- Diff: server/tserver/src/main/java/org/apache/accumulo/tserver/tablet/Tablet.java
---
    @@ -715,47 +722,54 @@ Batch nextBatch(SortedKeyValueIterator<Key,Value> iter, Range
range, int num, Se
     
         long maxResultsSize = tableConfiguration.getAsBytes(Property.TABLE_SCAN_MAXMEM);
     
    -    if (columns.size() == 0) {
    -      iter.seek(range, LocalityGroupUtil.EMPTY_CF_SET, false);
    -    } else {
    -      iter.seek(range, LocalityGroupUtil.families(columns), true);
    -    }
    -
         Key continueKey = null;
         boolean skipContinueKey = false;
     
         boolean endOfTabletReached = false;
    -    while (iter.hasTop()) {
     
    -      value = iter.getTopValue();
    -      key = iter.getTopKey();
    +    try {
    +      if (columns.size() == 0) {
    +        iter.seek(range, LocalityGroupUtil.EMPTY_CF_SET, false);
    --- End diff --
    
    > The underlying iterator would presumably seek its source with the same range. Now
the seek is iterating through keys past the initial start of the range and realizes at some
point that it has been passing by many of the underlying keys for whatever reason without
returning control to the Tablet. Now it throws a ScanYieldException specifying where it left
off in the seek.
    
    Yup, right.
    
    > Later the Tablet will come back and rebuild the iterator and seek it using the new
start as specified. The seek can now continue doing its job until it actually finds a key
that is to be returned. Where in this scenario is a key missed?
    
    The caveat here is that _no_ keys from this Range were returned; the SKVI would return
a "position" that is the same as the startKey of the Range is was passed.
    
    ```java
    +    } catch (ScanYieldException sye) {
    +      continueKey = new Key(sye.getPosition());
    +      skipContinueKey = true;
    +    }
    ```
    
    I may be grasping at straws, but, the little bit I've read about how this method is invoked,
the fact that `skipContinueKey` is set to true means that the next time we return to the Range,
we construct a new Range which is non-inclusive of the original start key.
    
    Concretely:
    
    `seek([a, z])` throwing `ScanYieldException` would net a call `seek((a, z])` (instead
of `seek([a,z])` again). This worried me because your test case obviously didn't cover this
path. If you are sure this is not an issue, I trust you.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

Mime
View raw message