----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://review.cloudera.org/r/1261/#review2009 -----------------------------------------------------------
Ship it! Looks good to me. Some comments below. branches/0.90/src/main/java/org/apache/hadoop/hbase/io/hfile/BlockCache.java <http://review.cloudera.org/r/1261/#comment6343> This looks like useful addition. branches/0.90/src/main/java/org/apache/hadoop/hbase/io/hfile/HFile.java <http://review.cloudera.org/r/1261/#comment6346> Why the flush? branches/0.90/src/main/java/org/apache/hadoop/hbase/io/hfile/HFile.java <http://review.cloudera.org/r/1261/#comment6344> Does this create new byte array? branches/0.90/src/main/java/org/apache/hadoop/hbase/io/hfile/HFile.java <http://review.cloudera.org/r/1261/#comment6345> I wonder if we have to have full path here? Anything less could cause clashes? But small optimization would strip the hbase.root at least? branches/0.90/src/main/java/org/apache/hadoop/hbase/io/hfile/HFile.java <http://review.cloudera.org/r/1261/#comment6347> Can you presize the BAOS? Whats the default? 4k? If so, and our default block size is 64k, that'd be a bit of expensive array resizing going on? Just guessing. branches/0.90/src/main/java/org/apache/hadoop/hbase/io/hfile/HFile.java <http://review.cloudera.org/r/1261/#comment6348> Surround with if debug? - stack On 2010-11-29 23:22:38, Jonathan Gray wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > http://review.cloudera.org/r/1261/ > ----------------------------------------------------------- > > (Updated 2010-11-29 23:22:38) > > > Review request for hbase, stack and khemani. > > > Summary > ------- > > This issue is about adding configuration options to add/remove from the block > cache when creating/closing files. For use cases with lots of flushing and > compacting, this might be desirable to prevent cache misses and maximize the > effective utilization of total block cache capacity. > > The first option, hbase.rs.cacheblocksonwrite, will make it so we pre-cache > blocks as we are writing out new files. > > The second option, hbase.rs.evictblocksonclose, will make it so we evict > blocks when files are closed. > > > This addresses bug HBASE-3287. > http://issues.apache.org/jira/browse/HBASE-3287 > > > Diffs > ----- > > > branches/0.90/src/main/java/org/apache/hadoop/hbase/io/HalfStoreFileReader.java > 1040422 > > branches/0.90/src/main/java/org/apache/hadoop/hbase/io/hfile/BlockCache.java > 1040422 > branches/0.90/src/main/java/org/apache/hadoop/hbase/io/hfile/HFile.java > 1040422 > > branches/0.90/src/main/java/org/apache/hadoop/hbase/io/hfile/LruBlockCache.java > 1040422 > > branches/0.90/src/main/java/org/apache/hadoop/hbase/io/hfile/SimpleBlockCache.java > 1040422 > > branches/0.90/src/main/java/org/apache/hadoop/hbase/mapreduce/LoadIncrementalHFiles.java > 1040422 > branches/0.90/src/main/java/org/apache/hadoop/hbase/regionserver/Store.java > 1040422 > > branches/0.90/src/main/java/org/apache/hadoop/hbase/regionserver/StoreFile.java > 1040422 > > branches/0.90/src/main/java/org/apache/hadoop/hbase/util/CompressionTest.java > 1040422 > > branches/0.90/src/test/java/org/apache/hadoop/hbase/HFilePerformanceEvaluation.java > 1040422 > > branches/0.90/src/test/java/org/apache/hadoop/hbase/io/TestHalfStoreFileReader.java > 1040422 > > branches/0.90/src/test/java/org/apache/hadoop/hbase/io/hfile/RandomSeek.java > 1040422 > branches/0.90/src/test/java/org/apache/hadoop/hbase/io/hfile/TestHFile.java > 1040422 > > branches/0.90/src/test/java/org/apache/hadoop/hbase/io/hfile/TestHFilePerformance.java > 1040422 > > branches/0.90/src/test/java/org/apache/hadoop/hbase/io/hfile/TestHFileSeek.java > 1040422 > > branches/0.90/src/test/java/org/apache/hadoop/hbase/io/hfile/TestReseekTo.java > 1040422 > > branches/0.90/src/test/java/org/apache/hadoop/hbase/io/hfile/TestSeekTo.java > 1040422 > > branches/0.90/src/test/java/org/apache/hadoop/hbase/mapreduce/TestLoadIncrementalHFiles.java > 1040422 > > branches/0.90/src/test/java/org/apache/hadoop/hbase/regionserver/TestStoreFile.java > 1040422 > > Diff: http://review.cloudera.org/r/1261/diff > > > Testing > ------- > > Added a unit test to TestStoreFile. That passes. > > Need to do perf testing on a cluster. > > > Thanks, > > Jonathan > >
