----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://review.cloudera.org/r/1263/#review2018 -----------------------------------------------------------
Ship it! This looks great. I love the test. There are some comments below. See what you think. I did not dig in deep on the algo but looks good. trunk/src/main/java/org/apache/hadoop/hbase/regionserver/Store.java <http://review.cloudera.org/r/1263/#comment6361> Good. I like the way you keep around old name. FYI, there's white space on end of some of these lines of yours. trunk/src/main/java/org/apache/hadoop/hbase/regionserver/Store.java <http://review.cloudera.org/r/1263/#comment6364> Is this right? We check all storefiles for references where before we only checked the subset of candidate compaction files for references? (Hmm.. maybe the old stuff was wrong?) trunk/src/main/java/org/apache/hadoop/hbase/regionserver/Store.java <http://review.cloudera.org/r/1263/#comment6362> White space trunk/src/main/java/org/apache/hadoop/hbase/regionserver/Store.java <http://review.cloudera.org/r/1263/#comment6365> Good trunk/src/main/java/org/apache/hadoop/hbase/regionserver/Store.java <http://review.cloudera.org/r/1263/#comment6366> I don't grok this comment trunk/src/main/java/org/apache/hadoop/hbase/regionserver/Store.java <http://review.cloudera.org/r/1263/#comment6367> So, its ok to mess w/ file order? We won't get ourselves into trouble if we don't respect the order in which files were written? We do a merge sort when we read all compaction candidates in so should be fine I suppose -- since its same as how scanner merges them...... Just asking because in old days order was important but I suppose we let go of that a while back? trunk/src/main/java/org/apache/hadoop/hbase/regionserver/Store.java <http://review.cloudera.org/r/1263/#comment6368> Is this a good name for this method? We're compacting a Store, not Stores, right? trunk/src/main/java/org/apache/hadoop/hbase/regionserver/StoreFile.java <http://review.cloudera.org/r/1263/#comment6369> Nice trunk/src/test/java/org/apache/hadoop/hbase/regionserver/TestCompactSelection.java <http://review.cloudera.org/r/1263/#comment6370> Excellent! I love you mocking up StoreFiles rather than fire up minicluster FYI... loads of white space in here. - stack On 2010-11-30 23:21:26, Nicolas wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > http://review.cloudera.org/r/1263/ > ----------------------------------------------------------- > > (Updated 2010-11-30 23:21:26) > > > Review request for hbase. > > > Summary > ------- > > Add ability to specify a maximum storefile size for compaction. After this > limit, we will not include this file in compactions. This is useful for large > object stores and clusters that pre-split regions. > > > This addresses bug HBASE-3290. > http://issues.apache.org/jira/browse/HBASE-3290 > > > Diffs > ----- > > trunk/src/main/java/org/apache/hadoop/hbase/regionserver/Store.java 1040878 > trunk/src/main/java/org/apache/hadoop/hbase/regionserver/StoreFile.java > 1040878 > > trunk/src/test/java/org/apache/hadoop/hbase/regionserver/TestCompactSelection.java > PRE-CREATION > > Diff: http://review.cloudera.org/r/1263/diff > > > Testing > ------- > > mvn test -Dtest=TestCompactSelection > mvn test -Dtest=TestCompaction > mvn test -Dtest=TestFromClientSide > mvn test > > cluster testing > > > Thanks, > > Nicolas > >
