hbase-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Todd Lipcon" <t...@cloudera.com>
Subject Re: Review Request: HBASE-2468: Improvements to prewarm META cache on clients.
Date Tue, 01 Jun 2010 04:52:12 GMT

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://review.hbase.org/r/98/#review107
-----------------------------------------------------------



src/main/java/org/apache/hadoop/hbase/client/HConnectionManager.java
<http://review.hbase.org/r/98/#comment635>

    typo: locationS



src/main/java/org/apache/hadoop/hbase/client/HConnectionManager.java
<http://review.hbase.org/r/98/#comment633>

    I think any of getCachedRegionCount, countCachedRegions, or getNumCachedRegions would
be a clearer name.



src/main/java/org/apache/hadoop/hbase/client/HConnectionManager.java
<http://review.hbase.org/r/98/#comment634>

    style-wise, why "final" here?



src/main/java/org/apache/hadoop/hbase/client/HConnectionManager.java
<http://review.hbase.org/r/98/#comment636>

    remove these @param javadocs - they just take up space if the param names are self-explanatory
(which these are)
    
    same thing above



src/main/java/org/apache/hadoop/hbase/client/HConnectionManager.java
<http://review.hbase.org/r/98/#comment637>

    this needs a Comparator argument, otherwise it does object identity rather than bytewise
comparison of the table names



src/main/java/org/apache/hadoop/hbase/client/HConnectionManager.java
<http://review.hbase.org/r/98/#comment640>

    javadoc out of date - it prefetches the region for the given row, and prefetchLimit regions
ahead



src/main/java/org/apache/hadoop/hbase/client/HConnectionManager.java
<http://review.hbase.org/r/98/#comment638>

    should return false to stop scanning at this point, right?



src/main/java/org/apache/hadoop/hbase/client/HConnectionManager.java
<http://review.hbase.org/r/98/#comment639>

    // cache this meta entry
    
    (it's not caching all)



src/main/java/org/apache/hadoop/hbase/client/HConnectionManager.java
<http://review.hbase.org/r/98/#comment641>

    this block should go after the cache check below, right?



src/main/java/org/apache/hadoop/hbase/client/HConnectionManager.java
<http://review.hbase.org/r/98/#comment647>

    this function needs to synchronized on cachedRegionLocations which is an unsynchronized
HashMap



src/main/java/org/apache/hadoop/hbase/client/HConnectionManager.java
<http://review.hbase.org/r/98/#comment648>

    return location != null;



src/main/java/org/apache/hadoop/hbase/client/HConnectionManager.java
<http://review.hbase.org/r/98/#comment654>

    kill this function



src/main/java/org/apache/hadoop/hbase/client/HTable.java
<http://review.hbase.org/r/98/#comment649>

    you should prefix the file with writeInt(allRegions.size()) so you don't have to use EOFException
catching below



src/main/java/org/apache/hadoop/hbase/client/HTable.java
<http://review.hbase.org/r/98/#comment650>

    yuck, see above



src/main/java/org/apache/hadoop/hbase/client/HTable.java
<http://review.hbase.org/r/98/#comment651>

    I feel like it would be cleaner to have the following methods be non-static, so they don't
need any arguments. that would reduce the number of functions by factor of two



src/main/java/org/apache/hadoop/hbase/client/HTable.java
<http://review.hbase.org/r/98/#comment653>

    incorrect javadoc, also a few places below



src/main/java/org/apache/hadoop/hbase/client/HTable.java
<http://review.hbase.org/r/98/#comment652>

    why do we need a separate function for enabled and disabled? aren't they always inverse
of each other?



src/main/java/org/apache/hadoop/hbase/client/MetaScanner.java
<http://review.hbase.org/r/98/#comment642>

    should make clear this is the row name in the user table, not the meta table.
    
    should also be clarified that it will start scanning with the region *after* row, because
it doesn't use getClosestRowBefore



src/test/java/org/apache/hadoop/hbase/client/TestFromClientSide.java
<http://review.hbase.org/r/98/#comment643>

    useless @throws



src/test/java/org/apache/hadoop/hbase/client/TestFromClientSide.java
<http://review.hbase.org/r/98/#comment645>

    you should use util.getTestDir here



src/test/java/org/apache/hadoop/hbase/client/TestFromClientSide.java
<http://review.hbase.org/r/98/#comment644>

    import java.io.File



src/test/java/org/apache/hadoop/hbase/client/TestFromClientSide.java
<http://review.hbase.org/r/98/#comment646>

    I really like these unit tests, but I think you should use a row key for the Get that
isn't also a region start key, as it may expose different behavior. eg I think you will end
up with 11 cached regions instead of 10


- Todd


On 2010-05-31 19:49:20, Mingjie Lai wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://review.hbase.org/r/98/
> -----------------------------------------------------------
> 
> (Updated 2010-05-31 19:49:20)
> 
> 
> Review request for hbase.
> 
> 
> Summary
> -------
> 
> HBASE-2468: Improvements to prewarm META cache on clients.
> 
> Changes:
> 1. Add new HTable methods which support region info de/serialation, and region cache
prewarm: 
> - void serializeRegionInfo(): clients could perform a large scan for all the meta for
the table, serialize the meta to a file. MR job can ship a copy of the meta for the table
in the DistributedCache
> - Map<HRegionInfo, HServerAddress> deserializeRegionInfo(): MR job can deserialize
the region info from the DistributedCache 
> - prewarmRegionCache(Map<HRegionInfo, HServerAddress> regionMap): MR job can prewarm
local region cache by the deserialized region info.
> 
> 2. For each client, each region cache read-miss could trigger read-ahead some number
of rows in META. This option could be turned on and off for one particular table. 
> 
> 
> This addresses bug HBASE-2468.
>     http://issues.apache.org/jira/browse/HBASE-2468
> 
> 
> Diffs
> -----
> 
>   src/main/java/org/apache/hadoop/hbase/client/HConnection.java 853164d 
>   src/main/java/org/apache/hadoop/hbase/client/HConnectionManager.java 09de2ac 
>   src/main/java/org/apache/hadoop/hbase/client/HTable.java 7ec95cb 
>   src/main/java/org/apache/hadoop/hbase/client/MetaScanner.java 3de661e 
>   src/test/java/org/apache/hadoop/hbase/client/TestFromClientSide.java 95e494a 
> 
> Diff: http://review.hbase.org/r/98/diff
> 
> 
> Testing
> -------
> 
> Unit tests passed locally for me. 
> 
> 
> Thanks,
> 
> Mingjie
> 
>


Mime
View raw message