HBase
  1. HBase
  2. HBASE-2792

Create a better way to chain log cleaners

    Details

    • Type: Improvement Improvement
    • Status: Resolved
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 0.90.0
    • Component/s: None
    • Labels:
      None
    • Hadoop Flags:
      Reviewed

      Description

      From Stack's review of HBASE-2223:

      Why this implementation have to know about other implementations? Can't we do a chain of decision classes? Any class can say no? As soon as any decision class says no, we exit the chain.... So in this case, first on the chain would be the ttl decision... then would be this one... and third would be the snapshotting decision. You don't have to do the chain as part of this patch but please open an issue to implement.

        Activity

        Hide
        Li Chongxin added a comment -

        here is some discussion about this issue in private mail:

        My idea is to replace the LogCleanerDelegate in OldLogsCleaner with a list of LogCleanerDelegate. A new LogCleanerDelegate is added to the list if it is required. Then the log file is checked against each LogCleanerDelegate in the list. It is deletable only if all the LogCleanerDelegate pass. For later dev who wants to provide a new LogCleanerDelegate, all he has to do is to implement the LogCleanerDelegate and add it to the list.

        From Jean-Daniel's reply:

        I think what you described sounds good, but I would add the requirement that anybody should be able to add their own implementations without changing the code (the new class would need to be on HBase's classpath). I think this could be done via the configuration file, like a comma separated list of fully qualified class names.

        Show
        Li Chongxin added a comment - here is some discussion about this issue in private mail: My idea is to replace the LogCleanerDelegate in OldLogsCleaner with a list of LogCleanerDelegate. A new LogCleanerDelegate is added to the list if it is required. Then the log file is checked against each LogCleanerDelegate in the list. It is deletable only if all the LogCleanerDelegate pass. For later dev who wants to provide a new LogCleanerDelegate, all he has to do is to implement the LogCleanerDelegate and add it to the list. From Jean-Daniel's reply: I think what you described sounds good, but I would add the requirement that anybody should be able to add their own implementations without changing the code (the new class would need to be on HBase's classpath). I think this could be done via the configuration file, like a comma separated list of fully qualified class names.
        Hide
        Li Chongxin added a comment -

        SnapshotLogCleaner is not included in this patch because it is dependent on the snapshot of hbase (https://issues.apache.org/jira/browse/HBASE-50), which is coming soon.

        Show
        Li Chongxin added a comment - SnapshotLogCleaner is not included in this patch because it is dependent on the snapshot of hbase ( https://issues.apache.org/jira/browse/HBASE-50 ), which is coming soon.
        Hide
        HBase Review Board added a comment -

        Message from: "Chongxin Li" <lichongxin@zju.edu.cn>

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

        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

        Show
        HBase Review Board added a comment - Message from: "Chongxin Li" <lichongxin@zju.edu.cn> ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://review.hbase.org/r/372/ ----------------------------------------------------------- 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
        Hide
        HBase Review Board added a comment -

        Message from: "Chongxin Li" <lichongxin@zju.edu.cn>

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

        (Updated 2010-07-22 23:00:05.908203)

        Review request for hbase.

        Changes
        -------

        HBASE-2792: Create a better way to chain log cleaners

        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 (updated)


        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

        Show
        HBase Review Board added a comment - Message from: "Chongxin Li" <lichongxin@zju.edu.cn> ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://review.hbase.org/r/372/ ----------------------------------------------------------- (Updated 2010-07-22 23:00:05.908203) Review request for hbase. Changes ------- HBASE-2792 : Create a better way to chain log cleaners 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 (updated) 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
        Hide
        HBase Review Board added a comment -

        Message from: stack@duboce.net

        -----------------------------------------------------------
        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
        Show
        HBase Review Board added a comment - Message from: stack@duboce.net ----------------------------------------------------------- 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
        Hide
        HBase Review Board added a comment -

        Message from: "Chongxin Li" <lichongxin@zju.edu.cn>

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

        (Updated 2010-07-24 05:10:08.798554)

        Review request for hbase.

        Changes
        -------

        Since currently there are only two log cleaners (TimeToLiveLogCleaner and ReplicationLogCleaner) in the chain, there would be 3 scenarios, that is log file doesn't pass the first log cleaner; log file passes the first log cleaner but is rejected by the second; log file passes both log cleaners and is then deleted. I think these 3 cases are all covered by the unit test. I've added some comments to the unit test to explain these 3 cases.

        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 (updated)


        src/main/java/org/apache/hadoop/hbase/master/LogCleanerDelegate.java 3ca3611
        src/main/java/org/apache/hadoop/hbase/master/LogsCleaner.java 37b2c3c
        src/main/java/org/apache/hadoop/hbase/master/ServerManager.java 9fb1cce
        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/TestLogsCleaner.java a92e0da

        Diff: http://review.hbase.org/r/372/diff

        Testing
        -------

        Unit test TestOldLogsCleaner passed.

        Thanks,

        Chongxin

        Show
        HBase Review Board added a comment - Message from: "Chongxin Li" <lichongxin@zju.edu.cn> ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://review.hbase.org/r/372/ ----------------------------------------------------------- (Updated 2010-07-24 05:10:08.798554) Review request for hbase. Changes ------- Since currently there are only two log cleaners (TimeToLiveLogCleaner and ReplicationLogCleaner) in the chain, there would be 3 scenarios, that is log file doesn't pass the first log cleaner; log file passes the first log cleaner but is rejected by the second; log file passes both log cleaners and is then deleted. I think these 3 cases are all covered by the unit test. I've added some comments to the unit test to explain these 3 cases. 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 (updated) src/main/java/org/apache/hadoop/hbase/master/LogCleanerDelegate.java 3ca3611 src/main/java/org/apache/hadoop/hbase/master/LogsCleaner.java 37b2c3c src/main/java/org/apache/hadoop/hbase/master/ServerManager.java 9fb1cce 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/TestLogsCleaner.java a92e0da Diff: http://review.hbase.org/r/372/diff Testing ------- Unit test TestOldLogsCleaner passed. Thanks, Chongxin
        Hide
        HBase Review Board added a comment -

        Message from: stack@duboce.net

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

        Ship it!

        +1

        Will commit in a little while.

        • stack
        Show
        HBase Review Board added a comment - Message from: stack@duboce.net ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://review.hbase.org/r/372/#review496 ----------------------------------------------------------- Ship it! +1 Will commit in a little while. stack
        Hide
        stack added a comment -

        Your patch won't apply Li. Can you redo the patch? Please include in your patch the rename of OldLogsCleaner to LogsCleaner if you can (not important if you can't).

        I did the rename myself before applying the patch but the LogsCleaner changes fail to apply anyways.

        Thanks.

        Show
        stack added a comment - Your patch won't apply Li. Can you redo the patch? Please include in your patch the rename of OldLogsCleaner to LogsCleaner if you can (not important if you can't). I did the rename myself before applying the patch but the LogsCleaner changes fail to apply anyways. Thanks.
        Hide
        HBase Review Board added a comment -

        Message from: "Chongxin Li" <lichongxin@zju.edu.cn>

        -----------------------------------------------------------
        This is an automatically generated e-mail. To reply, visit:
        http://review.cloudera.org/r/372/
        -----------------------------------------------------------

        (Updated 2010-07-29 06:51:56.166539)

        Review request for hbase.

        Changes
        -------

        I've tested this path. This can be applied.

        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 (updated)


        src/main/java/org/apache/hadoop/hbase/master/LogCleanerDelegate.java 3ca3611
        src/main/java/org/apache/hadoop/hbase/master/LogsCleaner.java PRE-CREATION
        src/main/java/org/apache/hadoop/hbase/master/OldLogsCleaner.java 37b2c3c
        src/main/java/org/apache/hadoop/hbase/master/ServerManager.java 9fb1cce
        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/TestLogsCleaner.java PRE-CREATION
        src/test/java/org/apache/hadoop/hbase/master/TestOldLogsCleaner.java a92e0da

        Diff: http://review.cloudera.org/r/372/diff

        Testing
        -------

        Unit test TestOldLogsCleaner passed.

        Thanks,

        Chongxin

        Show
        HBase Review Board added a comment - Message from: "Chongxin Li" <lichongxin@zju.edu.cn> ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://review.cloudera.org/r/372/ ----------------------------------------------------------- (Updated 2010-07-29 06:51:56.166539) Review request for hbase. Changes ------- I've tested this path. This can be applied. 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 (updated) src/main/java/org/apache/hadoop/hbase/master/LogCleanerDelegate.java 3ca3611 src/main/java/org/apache/hadoop/hbase/master/LogsCleaner.java PRE-CREATION src/main/java/org/apache/hadoop/hbase/master/OldLogsCleaner.java 37b2c3c src/main/java/org/apache/hadoop/hbase/master/ServerManager.java 9fb1cce 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/TestLogsCleaner.java PRE-CREATION src/test/java/org/apache/hadoop/hbase/master/TestOldLogsCleaner.java a92e0da Diff: http://review.cloudera.org/r/372/diff Testing ------- Unit test TestOldLogsCleaner passed. Thanks, Chongxin
        Hide
        stack added a comment -

        DIff copied from review.hbase.org, the diff I'm about to commit.

        Show
        stack added a comment - DIff copied from review.hbase.org, the diff I'm about to commit.
        Hide
        stack added a comment -

        Committed. Thanks for the patch Li Chongxin.

        Show
        stack added a comment - Committed. Thanks for the patch Li Chongxin.

          People

          • Assignee:
            Li Chongxin
            Reporter:
            Jean-Daniel Cryans
          • Votes:
            0 Vote for this issue
            Watchers:
            2 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development