scan can early exit for incrementColumnValue()

Review Request #1053 - Created Oct. 20, 2010 and updated

khemani
old-hbase
trunk
HBASE-3082
hbase
jgray
Ensure that during incrementColumnValue() the scan triggered by the get() does an early exit if it finds the KV in the memstore.
I have been testing it on my cluster. No unit testing yet.
  1. 
      
  2. Need to add apache licenses
  3. Maybe a little more description here.  Like, why do we need this.
    
    Also, is ControlledScan the best name for this class?  Not clear to me that this is "controlled" and thus the other scans are "uncontrolled"?
    
    If this was to be used for more later and even for this use, it's really additional configuration/options for Scans.  So is an InternalScan or ModifiedScan?
  4. Rather than these two public booleans, we should have getters/setters.
    
    And even rather than just straight set/get on each boolean, these two booleans are usually one or the other, right?  So I think it would be better to hide implementation detail and instead expose methods like setMemStoreOnly() and setFilesOnly() which would deal with the two booleans appropriately.  Then you'd have get methods like checkMemStore() and checkFiles() or something like that.
  5. Need to cleanup whitespace here.
    
    Maybe a little bit of javadoc for get_last_increment which describes what this does (and why it's still "correct" for increments)
  6. 
      
  1. Just some minor comments.
  2. This class doesn't need to be public, make it package-private.
  3. The style used in HBase mandates that this method be named getLastIncrement.
  4. A `//'-style comment would be better here
  5. use if (!results.isEmpty())
  6. Instead of two nested `if', you could combine both conditions with `&&'
  7. 
      
Review request changed
  1. 
      
  2. Why not put these into the base class?  Why create an entirely new class just to hold these?
    1. It is not added to the base class so as not to alter the public Scan API.
  3. Would this code be cleaner if we didnt have to use instanceof?
    1. yes, i agree it will be cleaner w/o instanceof. But in that case we will have to pass the flags all the way from HRegion to StoreScanner. A number of signatures will have to change.
      
      Another approach could be to only instantiate InternalScan in the server side. But again that is quite a bit of code change.
      
      Having an internal-scan should be useful. After all, a scan touches a lot of data and certain other tasks can potentially be piggybacked on a scan.
    2. ok that makes sense. let's go with it then.
  4. 
      
  1. 
      
  2. 
      
Loading...