hbase-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Jonathan Gray" <jg...@apache.org>
Subject Re: Review Request: scan can early exit for incrementColumnValue()
Date Wed, 20 Oct 2010 19:00:04 GMT

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://review.cloudera.org/r/1053/#review1585
-----------------------------------------------------------



src/main/java/org/apache/hadoop/hbase/regionserver/ControlledScan.java
<http://review.cloudera.org/r/1053/#comment5382>

    Need to add apache licenses



src/main/java/org/apache/hadoop/hbase/regionserver/ControlledScan.java
<http://review.cloudera.org/r/1053/#comment5383>

    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?



src/main/java/org/apache/hadoop/hbase/regionserver/ControlledScan.java
<http://review.cloudera.org/r/1053/#comment5385>

    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.



src/main/java/org/apache/hadoop/hbase/regionserver/ControlledScan.java
<http://review.cloudera.org/r/1053/#comment5384>

    Whitespace



src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java
<http://review.cloudera.org/r/1053/#comment5386>

    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)


- Jonathan


On 2010-10-20 11:45:51, khemani wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://review.cloudera.org/r/1053/
> -----------------------------------------------------------
> 
> (Updated 2010-10-20 11:45:51)
> 
> 
> Review request for hbase and Jonathan Gray.
> 
> 
> Summary
> -------
> 
> Ensure that during incrementColumnValue() the scan triggered by the get() does an early
exit if it finds the KV in the memstore.
> 
> 
> This addresses bug HBASE-3082.
>     http://issues.apache.org/jira/browse/HBASE-3082
> 
> 
> Diffs
> -----
> 
>   src/main/java/org/apache/hadoop/hbase/regionserver/ControlledScan.java PRE-CREATION

>   src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java 0e3940c 
>   src/main/java/org/apache/hadoop/hbase/regionserver/StoreScanner.java 4775fc8 
> 
> Diff: http://review.cloudera.org/r/1053/diff
> 
> 
> Testing
> -------
> 
> I have been testing it on my cluster. No unit testing yet.
> 
> 
> Thanks,
> 
> khemani
> 
>


Mime
View raw message