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