HBase, mail # dev - Re: Review Request: scan can early exit for incrementColumnValue()

Re: Review Request: scan can early exit for incrementColumnValue()

Jonathan Gray 2010-10-20, 19:00

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

Re: Review Request: scan can early exit for incrementColumnValue()

Benoit Sigoure 2010-10-20, 19:52

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://review.cloudera.org/r/1053/#review1586
-----------------------------------------------------------
Just some minor comments.
src/main/java/org/apache/hadoop/hbase/regionserver/ControlledScan.java
<http://review.cloudera.org/r/1053/#comment5387>

   This class doesn't need to be public, make it package-private.

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

   The style used in HBase mandates that this method be named getLastIncrement.

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

   A `//'-style comment would be better here

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

   use if (!results.isEmpty())

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

   Instead of two nested `if', you could combine both conditions with `&&'
- Benoit
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
>
>

Re: Review Request: scan can early exit for incrementColumnValue()

Ryan Rawson 2010-10-21, 04:41

Re: Review Request: scan can early exit for incrementColumnValue()

Ryan Rawson 2010-10-22, 22:44

> On 2010-10-20 21:41:23, Ryan Rawson wrote:
> > src/main/java/org/apache/hadoop/hbase/regionserver/StoreScanner.java, line 155
> > <http://review.cloudera.org/r/1053/diff/2/?file=15063#file15063line155>
> >
> > Would this code be cleaner if we didnt have to use instanceof?
>
> khemani wrote:
> 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.

ok that makes sense. let's go with it then.
- Ryan
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://review.cloudera.org/r/1053/#review1596
-----------------------------------------------------------
On 2010-10-20 21:31:17, khemani wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://review.cloudera.org/r/1053/
> -----------------------------------------------------------
>
> (Updated 2010-10-20 21:31:17)
>
>
> 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/HRegion.java 0e3940c
> src/main/java/org/apache/hadoop/hbase/regionserver/InternalScan.java PRE-CREATION
> 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
>
>

Re: Review Request: scan can early exit for incrementColumnValue()

Ryan Rawson 2010-10-22, 23:09

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

Ship it!
- Ryan
On 2010-10-20 21:31:17, khemani wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://review.cloudera.org/r/1053/
> -----------------------------------------------------------
>
> (Updated 2010-10-20 21:31:17)
>
>
> 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/HRegion.java 0e3940c
> src/main/java/org/apache/hadoop/hbase/regionserver/InternalScan.java PRE-CREATION
> 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
>
>