----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://review.hbase.org/r/372/#review479 -----------------------------------------------------------
Patch looks great. Can you add a little more to the unit test to prove your new chain mechanism works? I'd suggest that you remove all mention of snapshot from your patch (e.g. in the conf file below) so we can commit this infrastructural change without requiring snapshot code to be in place. In one of your snapshot patches to come, include edit to conf file to add back mention of snapshot. Good stuff Li. Make this standalone, and if you can, add some more to unit tests (its ok if you can't figure something but it'd be nice)... and then I'll commit. src/main/java/org/apache/hadoop/hbase/master/LogCleanerDelegate.java <http://review.hbase.org/r/372/#comment1971> This is nice. src/main/java/org/apache/hadoop/hbase/master/OldLogsCleaner.java <http://review.hbase.org/r/372/#comment1972> Please rename this class Li (even though you did not originally name it). OldLogsCleaner seems inappropriate; 'old' is not sufficient reason cleaning logs going forward. src/main/java/org/apache/hadoop/hbase/master/OldLogsCleaner.java <http://review.hbase.org/r/372/#comment1973> LogsCleaner is better than OldLogsCleaner I think. src/main/java/org/apache/hadoop/hbase/master/OldLogsCleaner.java <http://review.hbase.org/r/372/#comment1974> I think that is fine -- skipping if can't instantiate. - stack On 2010-07-22 23:00:05, Chongxin Li wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > http://review.hbase.org/r/372/ > ----------------------------------------------------------- > > (Updated 2010-07-22 23:00:05) > > > Review request for hbase. > > > Summary > ------- > > HBASE-2792: Create a better way to chain log cleaners > > > This addresses bug HBASE-2792. > http://issues.apache.org/jira/browse/HBASE-2792 > > > Diffs > ----- > > src/main/java/org/apache/hadoop/hbase/master/LogCleanerDelegate.java > 3ca3611 > src/main/java/org/apache/hadoop/hbase/master/OldLogsCleaner.java 37b2c3c > > src/main/java/org/apache/hadoop/hbase/replication/master/ReplicationLogCleaner.java > eb859aa > src/main/resources/hbase-default.xml e3a9669 > src/test/java/org/apache/hadoop/hbase/master/TestOldLogsCleaner.java > a92e0da > > Diff: http://review.hbase.org/r/372/diff > > > Testing > ------- > > Unit test TestOldLogsCleaner passed. > > > Thanks, > > Chongxin > >
