Details

    • Type: New Feature New Feature
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 3.3.3
    • Fix Version/s: 3.4.0
    • Component/s: server
    • Labels:
      None
    • Hadoop Flags:
      Reviewed

      Description

      I like to have ZK itself manage the amount of snapshots and logs kept, instead of relying on the PurgeTxnLog utility.

      1. ZOOKEEPER-1107.patch
        14 kB
        Laxman
      2. ZOOKEEPER-1107.6.patch
        18 kB
        Patrick Hunt
      3. ZOOKEEPER-1107.5.patch
        17 kB
        Laxman
      4. ZOOKEEPER-1107.5.patch
        17 kB
        Laxman
      5. ZOOKEEPER-1107.4.patch
        17 kB
        Laxman
      6. ZOOKEEPER-1107.3.patch
        15 kB
        Laxman
      7. ZOOKEEPER-1107.2.patch
        15 kB
        Laxman
      8. ZOOKEEPER-1107.1.patch
        16 kB
        Laxman

        Issue Links

          Activity

          Hide
          Patrick Hunt added a comment -

          Can you give some rational for this change? Is PTL not sufficient?

          Also see this JIRA, which would make such changes obsolete: ZOOKEEPER-323

          here are some reasons I can remember we do it this way:
          1) keep things simple, eg cron job with a basic script that could do whatever it likes re cleanup - thereby conforming to whatever requirements the operator might have (which will typ be very different from the concerns of the developers)
          2) don't cleanup unless the operator explicitly requests - ensures we don't lose data, say the operator is backing up every month, but the default is for zk to cleanup every hour (etc...)

          I don't see a problem with adding this as long as it's off by default, obv configurable through the zk config file. What sorts of functionality would be provided by this feature?

          Show
          Patrick Hunt added a comment - Can you give some rational for this change? Is PTL not sufficient? Also see this JIRA, which would make such changes obsolete: ZOOKEEPER-323 here are some reasons I can remember we do it this way: 1) keep things simple, eg cron job with a basic script that could do whatever it likes re cleanup - thereby conforming to whatever requirements the operator might have (which will typ be very different from the concerns of the developers) 2) don't cleanup unless the operator explicitly requests - ensures we don't lose data, say the operator is backing up every month, but the default is for zk to cleanup every hour (etc...) I don't see a problem with adding this as long as it's off by default, obv configurable through the zk config file. What sorts of functionality would be provided by this feature?
          Hide
          Jun Rao added a comment -

          There is nothing wrong with having a cron job. It just that there is one more command and one more job for the ops to remember.

          Show
          Jun Rao added a comment - There is nothing wrong with having a cron job. It just that there is one more command and one more job for the ops to remember.
          Hide
          Laxman added a comment -

          Snap and logs clean is a nice to have feature as part of zookeeper rather as a cron.

          I've considered the following points apart from the one mentioned by Jun.
          a) Cron jobs are difficult to debug in case of any issue.
          [Yet another process, Logs - Just my personal view]

          b) I would like to run cron job only when zookeeper is running. It will be difficult to manage two different processes [from an Operation Management perspective].

          c)

          don't cleanup unless the operator explicitly requests

          We can still maintain this by making it configurable.

          Correct me if my understanding is wrong.

          Following is the implementation I've in mind.
          a) Number of snapshots to retain and
          b) A purge task will be spawned at the startup.

          I can submit a patch based on suggestions.

          Show
          Laxman added a comment - Snap and logs clean is a nice to have feature as part of zookeeper rather as a cron. I've considered the following points apart from the one mentioned by Jun. a) Cron jobs are difficult to debug in case of any issue. [Yet another process, Logs - Just my personal view] b) I would like to run cron job only when zookeeper is running. It will be difficult to manage two different processes [from an Operation Management perspective] . c) don't cleanup unless the operator explicitly requests We can still maintain this by making it configurable. Correct me if my understanding is wrong. Following is the implementation I've in mind. a) Number of snapshots to retain and b) A purge task will be spawned at the startup. I can submit a patch based on suggestions.
          Hide
          Laxman added a comment -

          Attached the draft version of the patch.
          @Jun & Pat : Please review and give your suggestions to refine it further.

          Show
          Laxman added a comment - Attached the draft version of the patch. @Jun & Pat : Please review and give your suggestions to refine it further.
          Hide
          Hadoop QA added a comment -

          -1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12484646/ZOOKEEPER-1107.patch
          against trunk revision 1140017.

          +1 @author. The patch does not contain any @author tags.

          +1 tests included. The patch appears to include 3 new or modified tests.

          +1 javadoc. The javadoc tool did not generate any warning messages.

          -1 javac. The patch appears to cause tar ant target to fail.

          -1 findbugs. The patch appears to cause Findbugs (version 1.3.9) to fail.

          +1 release audit. The applied patch does not increase the total number of release audit warnings.

          -1 core tests. The patch failed core unit tests.

          +1 contrib tests. The patch passed contrib unit tests.

          Test results: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/356//testReport/
          Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/356//console

          This message is automatically generated.

          Show
          Hadoop QA added a comment - -1 overall. Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12484646/ZOOKEEPER-1107.patch against trunk revision 1140017. +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 3 new or modified tests. +1 javadoc. The javadoc tool did not generate any warning messages. -1 javac. The patch appears to cause tar ant target to fail. -1 findbugs. The patch appears to cause Findbugs (version 1.3.9) to fail. +1 release audit. The applied patch does not increase the total number of release audit warnings. -1 core tests. The patch failed core unit tests. +1 contrib tests. The patch passed contrib unit tests. Test results: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/356//testReport/ Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/356//console This message is automatically generated.
          Hide
          Laxman added a comment -

          Updated the patch for the following
          a) Cleared the findbugs noticed in testcode.
          b) Updated the zoo_sample.cfg
          c) Cleared javac warnings.

          Show
          Laxman added a comment - Updated the patch for the following a) Cleared the findbugs noticed in testcode. b) Updated the zoo_sample.cfg c) Cleared javac warnings.
          Hide
          Hadoop QA added a comment -

          -1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12484758/ZOOKEEPER-1107.1.patch
          against trunk revision 1140017.

          +1 @author. The patch does not contain any @author tags.

          +1 tests included. The patch appears to include 3 new or modified tests.

          +1 javadoc. The javadoc tool did not generate any warning messages.

          +1 javac. The applied patch does not increase the total number of javac compiler warnings.

          +1 findbugs. The patch does not introduce any new Findbugs (version 1.3.9) warnings.

          +1 release audit. The applied patch does not increase the total number of release audit warnings.

          -1 core tests. The patch failed core unit tests.

          +1 contrib tests. The patch passed contrib unit tests.

          Test results: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/361//testReport/
          Findbugs warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/361//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
          Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/361//console

          This message is automatically generated.

          Show
          Hadoop QA added a comment - -1 overall. Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12484758/ZOOKEEPER-1107.1.patch against trunk revision 1140017. +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 3 new or modified tests. +1 javadoc. The javadoc tool did not generate any warning messages. +1 javac. The applied patch does not increase the total number of javac compiler warnings. +1 findbugs. The patch does not introduce any new Findbugs (version 1.3.9) warnings. +1 release audit. The applied patch does not increase the total number of release audit warnings. -1 core tests. The patch failed core unit tests. +1 contrib tests. The patch passed contrib unit tests. Test results: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/361//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/361//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/361//console This message is automatically generated.
          Hide
          Hadoop QA added a comment -

          -1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12484774/ZOOKEEPER-1107.2.patch
          against trunk revision 1140017.

          +1 @author. The patch does not contain any @author tags.

          +1 tests included. The patch appears to include 3 new or modified tests.

          +1 javadoc. The javadoc tool did not generate any warning messages.

          +1 javac. The applied patch does not increase the total number of javac compiler warnings.

          +1 findbugs. The patch does not introduce any new Findbugs (version 1.3.9) warnings.

          +1 release audit. The applied patch does not increase the total number of release audit warnings.

          -1 core tests. The patch failed core unit tests.

          +1 contrib tests. The patch passed contrib unit tests.

          Test results: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/362//testReport/
          Findbugs warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/362//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
          Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/362//console

          This message is automatically generated.

          Show
          Hadoop QA added a comment - -1 overall. Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12484774/ZOOKEEPER-1107.2.patch against trunk revision 1140017. +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 3 new or modified tests. +1 javadoc. The javadoc tool did not generate any warning messages. +1 javac. The applied patch does not increase the total number of javac compiler warnings. +1 findbugs. The patch does not introduce any new Findbugs (version 1.3.9) warnings. +1 release audit. The applied patch does not increase the total number of release audit warnings. -1 core tests. The patch failed core unit tests. +1 contrib tests. The patch passed contrib unit tests. Test results: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/362//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/362//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/362//console This message is automatically generated.
          Hide
          Hadoop QA added a comment -

          -1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12484869/ZOOKEEPER-1107.3.patch
          against trunk revision 1141746.

          +1 @author. The patch does not contain any @author tags.

          +1 tests included. The patch appears to include 3 new or modified tests.

          +1 javadoc. The javadoc tool did not generate any warning messages.

          +1 javac. The applied patch does not increase the total number of javac compiler warnings.

          +1 findbugs. The patch does not introduce any new Findbugs (version 1.3.9) warnings.

          +1 release audit. The applied patch does not increase the total number of release audit warnings.

          -1 core tests. The patch failed core unit tests.

          +1 contrib tests. The patch passed contrib unit tests.

          Test results: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/365//testReport/
          Findbugs warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/365//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
          Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/365//console

          This message is automatically generated.

          Show
          Hadoop QA added a comment - -1 overall. Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12484869/ZOOKEEPER-1107.3.patch against trunk revision 1141746. +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 3 new or modified tests. +1 javadoc. The javadoc tool did not generate any warning messages. +1 javac. The applied patch does not increase the total number of javac compiler warnings. +1 findbugs. The patch does not introduce any new Findbugs (version 1.3.9) warnings. +1 release audit. The applied patch does not increase the total number of release audit warnings. -1 core tests. The patch failed core unit tests. +1 contrib tests. The patch passed contrib unit tests. Test results: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/365//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/365//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/365//console This message is automatically generated.
          Hide
          Laxman added a comment -

          -1 core tests. The patch failed core unit tests.

          There are no test failures in the report.
          https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/365//testReport/

          All Tests
          Package    Duration    Fail    (diff)    Skip    (diff)    Total    (diff)    
          org.apache.zookeeper  0.79 sec 0  0  19 +4 
          org.apache.zookeeper.server  9.1 sec 0  0  26  
          org.apache.zookeeper.server.quorum  1 min 45 sec 0  0  8  
          org.apache.zookeeper.test  14 min 0  0  272 -2 
          
          Show
          Laxman added a comment - -1 core tests. The patch failed core unit tests. There are no test failures in the report. https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/365//testReport/ All Tests Package Duration Fail (diff) Skip (diff) Total (diff) org.apache.zookeeper 0.79 sec 0 0 19 +4 org.apache.zookeeper.server 9.1 sec 0 0 26 org.apache.zookeeper.server.quorum 1 min 45 sec 0 0 8 org.apache.zookeeper.test 14 min 0 0 272 -2
          Hide
          Patrick Hunt added a comment -

          The C tests failed: (unfort they are not reported in the test results, you have to check the console output)

          [exec] [exec] make[1]: *** No rule to make target `tests/TestMulti.cc', needed by `zktest_st-TestMulti.o'. Stop.

          iirc this was due to a bad commit in the mainline - not you. I'll kick off another patch build for this.

          Show
          Patrick Hunt added a comment - The C tests failed: (unfort they are not reported in the test results, you have to check the console output) [exec] [exec] make [1] : *** No rule to make target `tests/TestMulti.cc', needed by `zktest_st-TestMulti.o'. Stop. iirc this was due to a bad commit in the mainline - not you. I'll kick off another patch build for this.
          Hide
          Hadoop QA added a comment -

          +1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12484869/ZOOKEEPER-1107.3.patch
          against trunk revision 1142377.

          +1 @author. The patch does not contain any @author tags.

          +1 tests included. The patch appears to include 3 new or modified tests.

          +1 javadoc. The javadoc tool did not generate any warning messages.

          +1 javac. The applied patch does not increase the total number of javac compiler warnings.

          +1 findbugs. The patch does not introduce any new Findbugs (version 1.3.9) warnings.

          +1 release audit. The applied patch does not increase the total number of release audit warnings.

          +1 core tests. The patch passed core unit tests.

          +1 contrib tests. The patch passed contrib unit tests.

          Test results: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/368//testReport/
          Findbugs warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/368//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
          Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/368//console

          This message is automatically generated.

          Show
          Hadoop QA added a comment - +1 overall. Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12484869/ZOOKEEPER-1107.3.patch against trunk revision 1142377. +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 3 new or modified tests. +1 javadoc. The javadoc tool did not generate any warning messages. +1 javac. The applied patch does not increase the total number of javac compiler warnings. +1 findbugs. The patch does not introduce any new Findbugs (version 1.3.9) warnings. +1 release audit. The applied patch does not increase the total number of release audit warnings. +1 core tests. The patch passed core unit tests. +1 contrib tests. The patch passed contrib unit tests. Test results: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/368//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/368//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/368//console This message is automatically generated.
          Hide
          jiraposter@reviews.apache.org added a comment -

          -----------------------------------------------------------
          This is an automatically generated e-mail. To reply, visit:
          https://reviews.apache.org/r/1043/
          -----------------------------------------------------------

          Review request for zookeeper, Patrick Hunt, Benjamin Reed, and Mahadev Konar.

          Summary
          -------

          I like to have ZK itself manage the amount of snapshots and logs kept, instead of relying on the PurgeTxnLog utility.

          This addresses bug ZOOKEEPER-1107.
          https://issues.apache.org/jira/browse/ZOOKEEPER-1107

          Diffs


          ./conf/zoo_sample.cfg 1141901
          ./src/java/main/org/apache/zookeeper/ZooKeeperPurger.java PRE-CREATION
          ./src/java/main/org/apache/zookeeper/server/quorum/QuorumPeerConfig.java 1141901
          ./src/java/test/org/apache/zookeeper/ZooKeeperPurgeTest.java PRE-CREATION

          Diff: https://reviews.apache.org/r/1043/diff

          Testing
          -------

          test added, passing hudson qa bot.

          Thanks,

          Patrick

          Show
          jiraposter@reviews.apache.org added a comment - ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/1043/ ----------------------------------------------------------- Review request for zookeeper, Patrick Hunt, Benjamin Reed, and Mahadev Konar. Summary ------- I like to have ZK itself manage the amount of snapshots and logs kept, instead of relying on the PurgeTxnLog utility. This addresses bug ZOOKEEPER-1107 . https://issues.apache.org/jira/browse/ZOOKEEPER-1107 Diffs ./conf/zoo_sample.cfg 1141901 ./src/java/main/org/apache/zookeeper/ZooKeeperPurger.java PRE-CREATION ./src/java/main/org/apache/zookeeper/server/quorum/QuorumPeerConfig.java 1141901 ./src/java/test/org/apache/zookeeper/ZooKeeperPurgeTest.java PRE-CREATION Diff: https://reviews.apache.org/r/1043/diff Testing ------- test added, passing hudson qa bot. Thanks, Patrick
          Hide
          jiraposter@reviews.apache.org added a comment -

          -----------------------------------------------------------
          This is an automatically generated e-mail. To reply, visit:
          https://reviews.apache.org/r/1043/#review1000
          -----------------------------------------------------------

          Are people just supposed to create their own new ZooKeeperPurger and call start on it? I don't see any hooks for starting this anywhere, or even a main method to use to start it. Would be nice to give that to people so they have a utility they can run easily.

          • Camille

          On 2011-07-07 23:10:13, Patrick Hunt wrote:

          -----------------------------------------------------------

          This is an automatically generated e-mail. To reply, visit:

          https://reviews.apache.org/r/1043/

          -----------------------------------------------------------

          (Updated 2011-07-07 23:10:13)

          Review request for zookeeper, Patrick Hunt, Benjamin Reed, and Mahadev Konar.

          Summary

          -------

          I like to have ZK itself manage the amount of snapshots and logs kept, instead of relying on the PurgeTxnLog utility.

          This addresses bug ZOOKEEPER-1107.

          https://issues.apache.org/jira/browse/ZOOKEEPER-1107

          Diffs

          -----

          ./conf/zoo_sample.cfg 1141901

          ./src/java/main/org/apache/zookeeper/ZooKeeperPurger.java PRE-CREATION

          ./src/java/main/org/apache/zookeeper/server/quorum/QuorumPeerConfig.java 1141901

          ./src/java/test/org/apache/zookeeper/ZooKeeperPurgeTest.java PRE-CREATION

          Diff: https://reviews.apache.org/r/1043/diff

          Testing

          -------

          test added, passing hudson qa bot.

          Thanks,

          Patrick

          Show
          jiraposter@reviews.apache.org added a comment - ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/1043/#review1000 ----------------------------------------------------------- Are people just supposed to create their own new ZooKeeperPurger and call start on it? I don't see any hooks for starting this anywhere, or even a main method to use to start it. Would be nice to give that to people so they have a utility they can run easily. Camille On 2011-07-07 23:10:13, Patrick Hunt wrote: ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/1043/ ----------------------------------------------------------- (Updated 2011-07-07 23:10:13) Review request for zookeeper, Patrick Hunt, Benjamin Reed, and Mahadev Konar. Summary ------- I like to have ZK itself manage the amount of snapshots and logs kept, instead of relying on the PurgeTxnLog utility. This addresses bug ZOOKEEPER-1107 . https://issues.apache.org/jira/browse/ZOOKEEPER-1107 Diffs ----- ./conf/zoo_sample.cfg 1141901 ./src/java/main/org/apache/zookeeper/ZooKeeperPurger.java PRE-CREATION ./src/java/main/org/apache/zookeeper/server/quorum/QuorumPeerConfig.java 1141901 ./src/java/test/org/apache/zookeeper/ZooKeeperPurgeTest.java PRE-CREATION Diff: https://reviews.apache.org/r/1043/diff Testing ------- test added, passing hudson qa bot. Thanks, Patrick
          Hide
          jiraposter@reviews.apache.org added a comment -

          -----------------------------------------------------------
          This is an automatically generated e-mail. To reply, visit:
          https://reviews.apache.org/r/1043/#review998
          -----------------------------------------------------------

          The documentation (src/docs) need to be updated - specifically the cleanup section in the admin guide.

          Have you considered hooking this into JMX or the 4letterwords? It would be nice for operators to get basic information. In JMX they could also control the settings... consider for a follow-on JIRA?

          ./conf/zoo_sample.cfg
          <https://reviews.apache.org/r/1043/#comment2060>

          My personal belief is that this should be turned off by default - i.e. comment out the parameters in the sample config.

          ./src/java/main/org/apache/zookeeper/ZooKeeperPurger.java
          <https://reviews.apache.org/r/1043/#comment2081>

          should this be in zookeeper or zookeeper.server ?

          ./src/java/main/org/apache/zookeeper/ZooKeeperPurger.java
          <https://reviews.apache.org/r/1043/#comment2080>

          consider calling this something more descriptive - perhaps "DatadirCleanupManager" or similar...

          ./src/java/main/org/apache/zookeeper/ZooKeeperPurger.java
          <https://reviews.apache.org/r/1043/#comment2069>

          enum?

          ./src/java/main/org/apache/zookeeper/ZooKeeperPurger.java
          <https://reviews.apache.org/r/1043/#comment2070>

          use TimeUnit

          ./src/java/main/org/apache/zookeeper/ZooKeeperPurger.java
          <https://reviews.apache.org/r/1043/#comment2076>

          perhaps this should be done in start?

          ./src/java/main/org/apache/zookeeper/ZooKeeperPurger.java
          <https://reviews.apache.org/r/1043/#comment2074>

          move this check to the start method.

          1) INFO level log if turned off
          2) exit the thread if turned off

          ./src/java/main/org/apache/zookeeper/ZooKeeperPurger.java
          <https://reviews.apache.org/r/1043/#comment2073>

          this formatting is not very nice, please adjust it a bit.

          ./src/java/main/org/apache/zookeeper/ZooKeeperPurger.java
          <https://reviews.apache.org/r/1043/#comment2075>

          I see tests, which is great, however where is this method being called in ZooKeeper server code proper? (what I mean is the server doesn't seem to be running this)

          ./src/java/main/org/apache/zookeeper/ZooKeeperPurger.java
          <https://reviews.apache.org/r/1043/#comment2079>

          given we are tracking the state shouldn't we be testing that here?

          ./src/java/main/org/apache/zookeeper/ZooKeeperPurger.java
          <https://reviews.apache.org/r/1043/#comment2077>

          I would rather we check if the state is started. (log warning if not)

          ./src/java/main/org/apache/zookeeper/ZooKeeperPurger.java
          <https://reviews.apache.org/r/1043/#comment2078>

          Zookeeper should be referred to as "ZooKeeper"

          ./src/java/main/org/apache/zookeeper/server/quorum/QuorumPeerConfig.java
          <https://reviews.apache.org/r/1043/#comment2067>

          specify explicit default, 3?

          ./src/java/main/org/apache/zookeeper/server/quorum/QuorumPeerConfig.java
          <https://reviews.apache.org/r/1043/#comment2068>

          specify explicit default - e.g. 0.

          ./src/java/main/org/apache/zookeeper/server/quorum/QuorumPeerConfig.java
          <https://reviews.apache.org/r/1043/#comment2065>

          the docs should reflect that this only controls the number of snaps to keep, the logs are purged based on the corresponding purged snaps.

          ./src/java/main/org/apache/zookeeper/server/quorum/QuorumPeerConfig.java
          <https://reviews.apache.org/r/1043/#comment2066>

          what does "time" refer to. elapsed time? what are the units.

          ./src/java/test/org/apache/zookeeper/ZooKeeperPurgeTest.java
          <https://reviews.apache.org/r/1043/#comment2082>

          Nice!

          • Patrick

          On 2011-07-07 23:10:13, Patrick Hunt wrote:

          -----------------------------------------------------------

          This is an automatically generated e-mail. To reply, visit:

          https://reviews.apache.org/r/1043/

          -----------------------------------------------------------

          (Updated 2011-07-07 23:10:13)

          Review request for zookeeper, Patrick Hunt, Benjamin Reed, and Mahadev Konar.

          Summary

          -------

          I like to have ZK itself manage the amount of snapshots and logs kept, instead of relying on the PurgeTxnLog utility.

          This addresses bug ZOOKEEPER-1107.

          https://issues.apache.org/jira/browse/ZOOKEEPER-1107

          Diffs

          -----

          ./conf/zoo_sample.cfg 1141901

          ./src/java/main/org/apache/zookeeper/ZooKeeperPurger.java PRE-CREATION

          ./src/java/main/org/apache/zookeeper/server/quorum/QuorumPeerConfig.java 1141901

          ./src/java/test/org/apache/zookeeper/ZooKeeperPurgeTest.java PRE-CREATION

          Diff: https://reviews.apache.org/r/1043/diff

          Testing

          -------

          test added, passing hudson qa bot.

          Thanks,

          Patrick

          Show
          jiraposter@reviews.apache.org added a comment - ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/1043/#review998 ----------------------------------------------------------- The documentation (src/docs) need to be updated - specifically the cleanup section in the admin guide. Have you considered hooking this into JMX or the 4letterwords? It would be nice for operators to get basic information. In JMX they could also control the settings... consider for a follow-on JIRA? ./conf/zoo_sample.cfg < https://reviews.apache.org/r/1043/#comment2060 > My personal belief is that this should be turned off by default - i.e. comment out the parameters in the sample config. ./src/java/main/org/apache/zookeeper/ZooKeeperPurger.java < https://reviews.apache.org/r/1043/#comment2081 > should this be in zookeeper or zookeeper.server ? ./src/java/main/org/apache/zookeeper/ZooKeeperPurger.java < https://reviews.apache.org/r/1043/#comment2080 > consider calling this something more descriptive - perhaps "DatadirCleanupManager" or similar... ./src/java/main/org/apache/zookeeper/ZooKeeperPurger.java < https://reviews.apache.org/r/1043/#comment2069 > enum? ./src/java/main/org/apache/zookeeper/ZooKeeperPurger.java < https://reviews.apache.org/r/1043/#comment2070 > use TimeUnit ./src/java/main/org/apache/zookeeper/ZooKeeperPurger.java < https://reviews.apache.org/r/1043/#comment2076 > perhaps this should be done in start? ./src/java/main/org/apache/zookeeper/ZooKeeperPurger.java < https://reviews.apache.org/r/1043/#comment2074 > move this check to the start method. 1) INFO level log if turned off 2) exit the thread if turned off ./src/java/main/org/apache/zookeeper/ZooKeeperPurger.java < https://reviews.apache.org/r/1043/#comment2073 > this formatting is not very nice, please adjust it a bit. ./src/java/main/org/apache/zookeeper/ZooKeeperPurger.java < https://reviews.apache.org/r/1043/#comment2075 > I see tests, which is great, however where is this method being called in ZooKeeper server code proper? (what I mean is the server doesn't seem to be running this) ./src/java/main/org/apache/zookeeper/ZooKeeperPurger.java < https://reviews.apache.org/r/1043/#comment2079 > given we are tracking the state shouldn't we be testing that here? ./src/java/main/org/apache/zookeeper/ZooKeeperPurger.java < https://reviews.apache.org/r/1043/#comment2077 > I would rather we check if the state is started. (log warning if not) ./src/java/main/org/apache/zookeeper/ZooKeeperPurger.java < https://reviews.apache.org/r/1043/#comment2078 > Zookeeper should be referred to as "ZooKeeper" ./src/java/main/org/apache/zookeeper/server/quorum/QuorumPeerConfig.java < https://reviews.apache.org/r/1043/#comment2067 > specify explicit default, 3? ./src/java/main/org/apache/zookeeper/server/quorum/QuorumPeerConfig.java < https://reviews.apache.org/r/1043/#comment2068 > specify explicit default - e.g. 0. ./src/java/main/org/apache/zookeeper/server/quorum/QuorumPeerConfig.java < https://reviews.apache.org/r/1043/#comment2065 > the docs should reflect that this only controls the number of snaps to keep, the logs are purged based on the corresponding purged snaps. ./src/java/main/org/apache/zookeeper/server/quorum/QuorumPeerConfig.java < https://reviews.apache.org/r/1043/#comment2066 > what does "time" refer to. elapsed time? what are the units. ./src/java/test/org/apache/zookeeper/ZooKeeperPurgeTest.java < https://reviews.apache.org/r/1043/#comment2082 > Nice! Patrick On 2011-07-07 23:10:13, Patrick Hunt wrote: ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/1043/ ----------------------------------------------------------- (Updated 2011-07-07 23:10:13) Review request for zookeeper, Patrick Hunt, Benjamin Reed, and Mahadev Konar. Summary ------- I like to have ZK itself manage the amount of snapshots and logs kept, instead of relying on the PurgeTxnLog utility. This addresses bug ZOOKEEPER-1107 . https://issues.apache.org/jira/browse/ZOOKEEPER-1107 Diffs ----- ./conf/zoo_sample.cfg 1141901 ./src/java/main/org/apache/zookeeper/ZooKeeperPurger.java PRE-CREATION ./src/java/main/org/apache/zookeeper/server/quorum/QuorumPeerConfig.java 1141901 ./src/java/test/org/apache/zookeeper/ZooKeeperPurgeTest.java PRE-CREATION Diff: https://reviews.apache.org/r/1043/diff Testing ------- test added, passing hudson qa bot. Thanks, Patrick
          Hide
          jiraposter@reviews.apache.org added a comment -

          On 2011-07-07 23:40:32, Patrick Hunt wrote:

          > ./src/java/main/org/apache/zookeeper/ZooKeeperPurger.java, lines 74-77

          > <https://reviews.apache.org/r/1043/diff/1/?file=22149#file22149line74>

          >

          > move this check to the start method.

          >

          > 1) INFO level log if turned off

          > 2) exit the thread if turned off

          Sorry, meant exit the start method if turned off (don't start the timer/task).

          • Patrick

          -----------------------------------------------------------
          This is an automatically generated e-mail. To reply, visit:
          https://reviews.apache.org/r/1043/#review998
          -----------------------------------------------------------

          On 2011-07-07 23:10:13, Patrick Hunt wrote:

          -----------------------------------------------------------

          This is an automatically generated e-mail. To reply, visit:

          https://reviews.apache.org/r/1043/

          -----------------------------------------------------------

          (Updated 2011-07-07 23:10:13)

          Review request for zookeeper, Patrick Hunt, Benjamin Reed, and Mahadev Konar.

          Summary

          -------

          I like to have ZK itself manage the amount of snapshots and logs kept, instead of relying on the PurgeTxnLog utility.

          This addresses bug ZOOKEEPER-1107.

          https://issues.apache.org/jira/browse/ZOOKEEPER-1107

          Diffs

          -----

          ./conf/zoo_sample.cfg 1141901

          ./src/java/main/org/apache/zookeeper/ZooKeeperPurger.java PRE-CREATION

          ./src/java/main/org/apache/zookeeper/server/quorum/QuorumPeerConfig.java 1141901

          ./src/java/test/org/apache/zookeeper/ZooKeeperPurgeTest.java PRE-CREATION

          Diff: https://reviews.apache.org/r/1043/diff

          Testing

          -------

          test added, passing hudson qa bot.

          Thanks,

          Patrick

          Show
          jiraposter@reviews.apache.org added a comment - On 2011-07-07 23:40:32, Patrick Hunt wrote: > ./src/java/main/org/apache/zookeeper/ZooKeeperPurger.java, lines 74-77 > < https://reviews.apache.org/r/1043/diff/1/?file=22149#file22149line74 > > > move this check to the start method. > > 1) INFO level log if turned off > 2) exit the thread if turned off Sorry, meant exit the start method if turned off (don't start the timer/task). Patrick ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/1043/#review998 ----------------------------------------------------------- On 2011-07-07 23:10:13, Patrick Hunt wrote: ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/1043/ ----------------------------------------------------------- (Updated 2011-07-07 23:10:13) Review request for zookeeper, Patrick Hunt, Benjamin Reed, and Mahadev Konar. Summary ------- I like to have ZK itself manage the amount of snapshots and logs kept, instead of relying on the PurgeTxnLog utility. This addresses bug ZOOKEEPER-1107 . https://issues.apache.org/jira/browse/ZOOKEEPER-1107 Diffs ----- ./conf/zoo_sample.cfg 1141901 ./src/java/main/org/apache/zookeeper/ZooKeeperPurger.java PRE-CREATION ./src/java/main/org/apache/zookeeper/server/quorum/QuorumPeerConfig.java 1141901 ./src/java/test/org/apache/zookeeper/ZooKeeperPurgeTest.java PRE-CREATION Diff: https://reviews.apache.org/r/1043/diff Testing ------- test added, passing hudson qa bot. Thanks, Patrick
          Hide
          jiraposter@reviews.apache.org added a comment -

          On 2011-07-07 23:34:13, Camille Fournier wrote:

          > Are people just supposed to create their own new ZooKeeperPurger and call start on it? I don't see any hooks for starting this anywhere, or even a main method to use to start it. Would be nice to give that to people so they have a utility they can run easily.

          Heh, you beat me to it. I think it should be started by the server, but the config defaults should have it turned off (time=0) by default. (more in my comments)

          • Patrick

          -----------------------------------------------------------
          This is an automatically generated e-mail. To reply, visit:
          https://reviews.apache.org/r/1043/#review1000
          -----------------------------------------------------------

          On 2011-07-07 23:10:13, Patrick Hunt wrote:

          -----------------------------------------------------------

          This is an automatically generated e-mail. To reply, visit:

          https://reviews.apache.org/r/1043/

          -----------------------------------------------------------

          (Updated 2011-07-07 23:10:13)

          Review request for zookeeper, Patrick Hunt, Benjamin Reed, and Mahadev Konar.

          Summary

          -------

          I like to have ZK itself manage the amount of snapshots and logs kept, instead of relying on the PurgeTxnLog utility.

          This addresses bug ZOOKEEPER-1107.

          https://issues.apache.org/jira/browse/ZOOKEEPER-1107

          Diffs

          -----

          ./conf/zoo_sample.cfg 1141901

          ./src/java/main/org/apache/zookeeper/ZooKeeperPurger.java PRE-CREATION

          ./src/java/main/org/apache/zookeeper/server/quorum/QuorumPeerConfig.java 1141901

          ./src/java/test/org/apache/zookeeper/ZooKeeperPurgeTest.java PRE-CREATION

          Diff: https://reviews.apache.org/r/1043/diff

          Testing

          -------

          test added, passing hudson qa bot.

          Thanks,

          Patrick

          Show
          jiraposter@reviews.apache.org added a comment - On 2011-07-07 23:34:13, Camille Fournier wrote: > Are people just supposed to create their own new ZooKeeperPurger and call start on it? I don't see any hooks for starting this anywhere, or even a main method to use to start it. Would be nice to give that to people so they have a utility they can run easily. Heh, you beat me to it. I think it should be started by the server, but the config defaults should have it turned off (time=0) by default. (more in my comments) Patrick ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/1043/#review1000 ----------------------------------------------------------- On 2011-07-07 23:10:13, Patrick Hunt wrote: ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/1043/ ----------------------------------------------------------- (Updated 2011-07-07 23:10:13) Review request for zookeeper, Patrick Hunt, Benjamin Reed, and Mahadev Konar. Summary ------- I like to have ZK itself manage the amount of snapshots and logs kept, instead of relying on the PurgeTxnLog utility. This addresses bug ZOOKEEPER-1107 . https://issues.apache.org/jira/browse/ZOOKEEPER-1107 Diffs ----- ./conf/zoo_sample.cfg 1141901 ./src/java/main/org/apache/zookeeper/ZooKeeperPurger.java PRE-CREATION ./src/java/main/org/apache/zookeeper/server/quorum/QuorumPeerConfig.java 1141901 ./src/java/test/org/apache/zookeeper/ZooKeeperPurgeTest.java PRE-CREATION Diff: https://reviews.apache.org/r/1043/diff Testing ------- test added, passing hudson qa bot. Thanks, Patrick
          Hide
          Laxman added a comment -

          Thanks for your review Pat and Camille. Soon, I will fix all your comments and upload new patch.

          Show
          Laxman added a comment - Thanks for your review Pat and Camille. Soon, I will fix all your comments and upload new patch.
          Hide
          Laxman added a comment -

          One more thing to be taken care.

          Show
          Laxman added a comment - One more thing to be taken care. Use slf4j in the new classes. ZOOKEEPER-850
          Hide
          Laxman added a comment -

          Attached the reworked patch after fixing the review comments.

          Show
          Laxman added a comment - Attached the reworked patch after fixing the review comments.
          Hide
          jiraposter@reviews.apache.org added a comment -

          On 2011-07-07 23:40:32, Patrick Hunt wrote:

          > The documentation (src/docs) need to be updated - specifically the cleanup section in the admin guide.

          >

          > Have you considered hooking this into JMX or the 4letterwords? It would be nice for operators to get basic information. In JMX they could also control the settings... consider for a follow-on JIRA?

          Fixed the documentation part.

          @Pat: I guess I'm not clear about how "hooking into JMX or 4 letter-words" will be helpful. Can you please explain this idea? I can takeup this task in separate JIRA.

          On 2011-07-07 23:40:32, Patrick Hunt wrote:

          > ./conf/zoo_sample.cfg, lines 15-21

          > <https://reviews.apache.org/r/1043/diff/1/?file=22148#file22148line15>

          >

          > My personal belief is that this should be turned off by default - i.e. comment out the parameters in the sample config.

          Fixed. Also, renamed the configuration keys to make them inline with existing.

          On 2011-07-07 23:40:32, Patrick Hunt wrote:

          > ./src/java/test/org/apache/zookeeper/ZooKeeperPurgeTest.java, line 37

          > <https://reviews.apache.org/r/1043/diff/1/?file=22151#file22151line37>

          >

          > Nice!

          Corrected code format issues.

          On 2011-07-07 23:40:32, Patrick Hunt wrote:

          > ./src/java/main/org/apache/zookeeper/server/quorum/QuorumPeerConfig.java, lines 384-385

          > <https://reviews.apache.org/r/1043/diff/1/?file=22150#file22150line384>

          >

          > what does "time" refer to. elapsed time? what are the units.

          Fixed.

          On 2011-07-07 23:40:32, Patrick Hunt wrote:

          > ./src/java/main/org/apache/zookeeper/server/quorum/QuorumPeerConfig.java, lines 374-375

          > <https://reviews.apache.org/r/1043/diff/1/?file=22150#file22150line374>

          >

          > the docs should reflect that this only controls the number of snaps to keep, the logs are purged based on the corresponding purged snaps.

          Fixed. Comments are moved to applicable class DatadirCleanupManager and also documented in admin guide as suggested.

          On 2011-07-07 23:40:32, Patrick Hunt wrote:

          > ./src/java/main/org/apache/zookeeper/server/quorum/QuorumPeerConfig.java, line 73

          > <https://reviews.apache.org/r/1043/diff/1/?file=22150#file22150line73>

          >

          > specify explicit default - e.g. 0.

          Fixed.

          On 2011-07-07 23:40:32, Patrick Hunt wrote:

          > ./src/java/main/org/apache/zookeeper/server/quorum/QuorumPeerConfig.java, line 72

          > <https://reviews.apache.org/r/1043/diff/1/?file=22150#file22150line72>

          >

          > specify explicit default, 3?

          Fixed.

          On 2011-07-07 23:40:32, Patrick Hunt wrote:

          > ./src/java/main/org/apache/zookeeper/ZooKeeperPurger.java, line 142

          > <https://reviews.apache.org/r/1043/diff/1/?file=22149#file22149line142>

          >

          > Zookeeper should be referred to as "ZooKeeper"

          Fixed. Redundant information in logs has been removed.

          On 2011-07-07 23:40:32, Patrick Hunt wrote:

          > ./src/java/main/org/apache/zookeeper/ZooKeeperPurger.java, line 118

          > <https://reviews.apache.org/r/1043/diff/1/?file=22149#file22149line118>

          >

          > I would rather we check if the state is started. (log warning if not)

          Fixed.

          On 2011-07-07 23:40:32, Patrick Hunt wrote:

          > ./src/java/main/org/apache/zookeeper/ZooKeeperPurger.java, line 104

          > <https://reviews.apache.org/r/1043/diff/1/?file=22149#file22149line104>

          >

          > given we are tracking the state shouldn't we be testing that here?

          Fixed.

          On 2011-07-07 23:40:32, Patrick Hunt wrote:

          > ./src/java/main/org/apache/zookeeper/ZooKeeperPurger.java, line 103

          > <https://reviews.apache.org/r/1043/diff/1/?file=22149#file22149line103>

          >

          > I see tests, which is great, however where is this method being called in ZooKeeper server code proper? (what I mean is the server doesn't seem to be running this)

          Called in QuorumPeerMain. Changes included in the latest patch.

          On 2011-07-07 23:40:32, Patrick Hunt wrote:

          > ./src/java/main/org/apache/zookeeper/ZooKeeperPurger.java, lines 83-86

          > <https://reviews.apache.org/r/1043/diff/1/?file=22149#file22149line83>

          >

          > this formatting is not very nice, please adjust it a bit.

          Fixed in all the places.

          On 2011-07-07 23:40:32, Patrick Hunt wrote:

          > ./src/java/main/org/apache/zookeeper/ZooKeeperPurger.java, lines 74-77

          > <https://reviews.apache.org/r/1043/diff/1/?file=22149#file22149line74>

          >

          > move this check to the start method.

          >

          > 1) INFO level log if turned off

          > 2) exit the thread if turned off

          Patrick Hunt wrote:

          Sorry, meant exit the start method if turned off (don't start the timer/task).

          Fixed.

          On 2011-07-07 23:40:32, Patrick Hunt wrote:

          > ./src/java/main/org/apache/zookeeper/ZooKeeperPurger.java, line 66

          > <https://reviews.apache.org/r/1043/diff/1/?file=22149#file22149line66>

          >

          > perhaps this should be done in start?

          Fixed.

          On 2011-07-07 23:40:32, Patrick Hunt wrote:

          > ./src/java/main/org/apache/zookeeper/ZooKeeperPurger.java, line 45

          > <https://reviews.apache.org/r/1043/diff/1/?file=22149#file22149line45>

          >

          > use TimeUnit

          Fixed.

          On 2011-07-07 23:40:32, Patrick Hunt wrote:

          > ./src/java/main/org/apache/zookeeper/ZooKeeperPurger.java, lines 39-43

          > <https://reviews.apache.org/r/1043/diff/1/?file=22149#file22149line39>

          >

          > enum?

          Introduced new enum PurgeTaskStatus.

          On 2011-07-07 23:40:32, Patrick Hunt wrote:

          > ./src/java/main/org/apache/zookeeper/ZooKeeperPurger.java, line 38

          > <https://reviews.apache.org/r/1043/diff/1/?file=22149#file22149line38>

          >

          > consider calling this something more descriptive - perhaps "DatadirCleanupManager" or similar...

          Renamed the source file and test file as well.

          On 2011-07-07 23:40:32, Patrick Hunt wrote:

          > ./src/java/main/org/apache/zookeeper/ZooKeeperPurger.java, line 19

          > <https://reviews.apache.org/r/1043/diff/1/?file=22149#file22149line19>

          >

          > should this be in zookeeper or zookeeper.server ?

          Moved both source and test files to zookeeper.server package.

          • Laxman

          -----------------------------------------------------------
          This is an automatically generated e-mail. To reply, visit:
          https://reviews.apache.org/r/1043/#review998
          -----------------------------------------------------------

          On 2011-07-07 23:10:13, Patrick Hunt wrote:

          -----------------------------------------------------------

          This is an automatically generated e-mail. To reply, visit:

          https://reviews.apache.org/r/1043/

          -----------------------------------------------------------

          (Updated 2011-07-07 23:10:13)

          Review request for zookeeper, Patrick Hunt, Benjamin Reed, and Mahadev Konar.

          Summary

          -------

          I like to have ZK itself manage the amount of snapshots and logs kept, instead of relying on the PurgeTxnLog utility.

          This addresses bug ZOOKEEPER-1107.

          https://issues.apache.org/jira/browse/ZOOKEEPER-1107

          Diffs

          -----

          ./conf/zoo_sample.cfg 1141901

          ./src/java/main/org/apache/zookeeper/ZooKeeperPurger.java PRE-CREATION

          ./src/java/main/org/apache/zookeeper/server/quorum/QuorumPeerConfig.java 1141901

          ./src/java/test/org/apache/zookeeper/ZooKeeperPurgeTest.java PRE-CREATION

          Diff: https://reviews.apache.org/r/1043/diff

          Testing

          -------

          test added, passing hudson qa bot.

          Thanks,

          Patrick

          Show
          jiraposter@reviews.apache.org added a comment - On 2011-07-07 23:40:32, Patrick Hunt wrote: > The documentation (src/docs) need to be updated - specifically the cleanup section in the admin guide. > > Have you considered hooking this into JMX or the 4letterwords? It would be nice for operators to get basic information. In JMX they could also control the settings... consider for a follow-on JIRA? Fixed the documentation part. @Pat: I guess I'm not clear about how "hooking into JMX or 4 letter-words" will be helpful. Can you please explain this idea? I can takeup this task in separate JIRA. On 2011-07-07 23:40:32, Patrick Hunt wrote: > ./conf/zoo_sample.cfg, lines 15-21 > < https://reviews.apache.org/r/1043/diff/1/?file=22148#file22148line15 > > > My personal belief is that this should be turned off by default - i.e. comment out the parameters in the sample config. Fixed. Also, renamed the configuration keys to make them inline with existing. On 2011-07-07 23:40:32, Patrick Hunt wrote: > ./src/java/test/org/apache/zookeeper/ZooKeeperPurgeTest.java, line 37 > < https://reviews.apache.org/r/1043/diff/1/?file=22151#file22151line37 > > > Nice! Corrected code format issues. On 2011-07-07 23:40:32, Patrick Hunt wrote: > ./src/java/main/org/apache/zookeeper/server/quorum/QuorumPeerConfig.java, lines 384-385 > < https://reviews.apache.org/r/1043/diff/1/?file=22150#file22150line384 > > > what does "time" refer to. elapsed time? what are the units. Fixed. On 2011-07-07 23:40:32, Patrick Hunt wrote: > ./src/java/main/org/apache/zookeeper/server/quorum/QuorumPeerConfig.java, lines 374-375 > < https://reviews.apache.org/r/1043/diff/1/?file=22150#file22150line374 > > > the docs should reflect that this only controls the number of snaps to keep, the logs are purged based on the corresponding purged snaps. Fixed. Comments are moved to applicable class DatadirCleanupManager and also documented in admin guide as suggested. On 2011-07-07 23:40:32, Patrick Hunt wrote: > ./src/java/main/org/apache/zookeeper/server/quorum/QuorumPeerConfig.java, line 73 > < https://reviews.apache.org/r/1043/diff/1/?file=22150#file22150line73 > > > specify explicit default - e.g. 0. Fixed. On 2011-07-07 23:40:32, Patrick Hunt wrote: > ./src/java/main/org/apache/zookeeper/server/quorum/QuorumPeerConfig.java, line 72 > < https://reviews.apache.org/r/1043/diff/1/?file=22150#file22150line72 > > > specify explicit default, 3? Fixed. On 2011-07-07 23:40:32, Patrick Hunt wrote: > ./src/java/main/org/apache/zookeeper/ZooKeeperPurger.java, line 142 > < https://reviews.apache.org/r/1043/diff/1/?file=22149#file22149line142 > > > Zookeeper should be referred to as "ZooKeeper" Fixed. Redundant information in logs has been removed. On 2011-07-07 23:40:32, Patrick Hunt wrote: > ./src/java/main/org/apache/zookeeper/ZooKeeperPurger.java, line 118 > < https://reviews.apache.org/r/1043/diff/1/?file=22149#file22149line118 > > > I would rather we check if the state is started. (log warning if not) Fixed. On 2011-07-07 23:40:32, Patrick Hunt wrote: > ./src/java/main/org/apache/zookeeper/ZooKeeperPurger.java, line 104 > < https://reviews.apache.org/r/1043/diff/1/?file=22149#file22149line104 > > > given we are tracking the state shouldn't we be testing that here? Fixed. On 2011-07-07 23:40:32, Patrick Hunt wrote: > ./src/java/main/org/apache/zookeeper/ZooKeeperPurger.java, line 103 > < https://reviews.apache.org/r/1043/diff/1/?file=22149#file22149line103 > > > I see tests, which is great, however where is this method being called in ZooKeeper server code proper? (what I mean is the server doesn't seem to be running this) Called in QuorumPeerMain. Changes included in the latest patch. On 2011-07-07 23:40:32, Patrick Hunt wrote: > ./src/java/main/org/apache/zookeeper/ZooKeeperPurger.java, lines 83-86 > < https://reviews.apache.org/r/1043/diff/1/?file=22149#file22149line83 > > > this formatting is not very nice, please adjust it a bit. Fixed in all the places. On 2011-07-07 23:40:32, Patrick Hunt wrote: > ./src/java/main/org/apache/zookeeper/ZooKeeperPurger.java, lines 74-77 > < https://reviews.apache.org/r/1043/diff/1/?file=22149#file22149line74 > > > move this check to the start method. > > 1) INFO level log if turned off > 2) exit the thread if turned off Patrick Hunt wrote: Sorry, meant exit the start method if turned off (don't start the timer/task). Fixed. On 2011-07-07 23:40:32, Patrick Hunt wrote: > ./src/java/main/org/apache/zookeeper/ZooKeeperPurger.java, line 66 > < https://reviews.apache.org/r/1043/diff/1/?file=22149#file22149line66 > > > perhaps this should be done in start? Fixed. On 2011-07-07 23:40:32, Patrick Hunt wrote: > ./src/java/main/org/apache/zookeeper/ZooKeeperPurger.java, line 45 > < https://reviews.apache.org/r/1043/diff/1/?file=22149#file22149line45 > > > use TimeUnit Fixed. On 2011-07-07 23:40:32, Patrick Hunt wrote: > ./src/java/main/org/apache/zookeeper/ZooKeeperPurger.java, lines 39-43 > < https://reviews.apache.org/r/1043/diff/1/?file=22149#file22149line39 > > > enum? Introduced new enum PurgeTaskStatus. On 2011-07-07 23:40:32, Patrick Hunt wrote: > ./src/java/main/org/apache/zookeeper/ZooKeeperPurger.java, line 38 > < https://reviews.apache.org/r/1043/diff/1/?file=22149#file22149line38 > > > consider calling this something more descriptive - perhaps "DatadirCleanupManager" or similar... Renamed the source file and test file as well. On 2011-07-07 23:40:32, Patrick Hunt wrote: > ./src/java/main/org/apache/zookeeper/ZooKeeperPurger.java, line 19 > < https://reviews.apache.org/r/1043/diff/1/?file=22149#file22149line19 > > > should this be in zookeeper or zookeeper.server ? Moved both source and test files to zookeeper.server package. Laxman ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/1043/#review998 ----------------------------------------------------------- On 2011-07-07 23:10:13, Patrick Hunt wrote: ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/1043/ ----------------------------------------------------------- (Updated 2011-07-07 23:10:13) Review request for zookeeper, Patrick Hunt, Benjamin Reed, and Mahadev Konar. Summary ------- I like to have ZK itself manage the amount of snapshots and logs kept, instead of relying on the PurgeTxnLog utility. This addresses bug ZOOKEEPER-1107 . https://issues.apache.org/jira/browse/ZOOKEEPER-1107 Diffs ----- ./conf/zoo_sample.cfg 1141901 ./src/java/main/org/apache/zookeeper/ZooKeeperPurger.java PRE-CREATION ./src/java/main/org/apache/zookeeper/server/quorum/QuorumPeerConfig.java 1141901 ./src/java/test/org/apache/zookeeper/ZooKeeperPurgeTest.java PRE-CREATION Diff: https://reviews.apache.org/r/1043/diff Testing ------- test added, passing hudson qa bot. Thanks, Patrick
          Hide
          Hadoop QA added a comment -

          +1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12486574/ZOOKEEPER-1107.4.patch
          against trunk revision 1146961.

          +1 @author. The patch does not contain any @author tags.

          +1 tests included. The patch appears to include 3 new or modified tests.

          +1 javadoc. The javadoc tool did not generate any warning messages.

          +1 javac. The applied patch does not increase the total number of javac compiler warnings.

          +1 findbugs. The patch does not introduce any new Findbugs (version 1.3.9) warnings.

          +1 release audit. The applied patch does not increase the total number of release audit warnings.

          +1 core tests. The patch passed core unit tests.

          +1 contrib tests. The patch passed contrib unit tests.

          Test results: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/396//testReport/
          Findbugs warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/396//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
          Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/396//console

          This message is automatically generated.

          Show
          Hadoop QA added a comment - +1 overall. Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12486574/ZOOKEEPER-1107.4.patch against trunk revision 1146961. +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 3 new or modified tests. +1 javadoc. The javadoc tool did not generate any warning messages. +1 javac. The applied patch does not increase the total number of javac compiler warnings. +1 findbugs. The patch does not introduce any new Findbugs (version 1.3.9) warnings. +1 release audit. The applied patch does not increase the total number of release audit warnings. +1 core tests. The patch passed core unit tests. +1 contrib tests. The patch passed contrib unit tests. Test results: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/396//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/396//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/396//console This message is automatically generated.
          Hide
          jiraposter@reviews.apache.org added a comment -

          On 2011-07-07 23:40:32, Patrick Hunt wrote:

          > The documentation (src/docs) need to be updated - specifically the cleanup section in the admin guide.

          >

          > Have you considered hooking this into JMX or the 4letterwords? It would be nice for operators to get basic information. In JMX they could also control the settings... consider for a follow-on JIRA?

          Laxman Ch wrote:

          Fixed the documentation part.

          @Pat: I guess I'm not clear about how "hooking into JMX or 4 letter-words" will be helpful. Can you please explain this idea? I can takeup this task in separate JIRA.

          both JMX and 4letterwords provide insight to the operator into the runtime status of the system
          http://zookeeper.apache.org/doc/r3.3.3/zookeeperAdmin.html#sc_zkCommands
          http://zookeeper.apache.org/doc/r3.3.3/zookeeperJMX.html

          what I was suggesting is that you could allow such insight into the cleanup task - for example is the process running, when the last time it ran, a history of the files cleaned up and when, stop the task, restart, etc...

          • Patrick

          -----------------------------------------------------------
          This is an automatically generated e-mail. To reply, visit:
          https://reviews.apache.org/r/1043/#review998
          -----------------------------------------------------------

          On 2011-07-07 23:10:13, Patrick Hunt wrote:

          -----------------------------------------------------------

          This is an automatically generated e-mail. To reply, visit:

          https://reviews.apache.org/r/1043/

          -----------------------------------------------------------

          (Updated 2011-07-07 23:10:13)

          Review request for zookeeper, Patrick Hunt, Benjamin Reed, and Mahadev Konar.

          Summary

          -------

          I like to have ZK itself manage the amount of snapshots and logs kept, instead of relying on the PurgeTxnLog utility.

          This addresses bug ZOOKEEPER-1107.

          https://issues.apache.org/jira/browse/ZOOKEEPER-1107

          Diffs

          -----

          ./conf/zoo_sample.cfg 1141901

          ./src/java/main/org/apache/zookeeper/ZooKeeperPurger.java PRE-CREATION

          ./src/java/main/org/apache/zookeeper/server/quorum/QuorumPeerConfig.java 1141901

          ./src/java/test/org/apache/zookeeper/ZooKeeperPurgeTest.java PRE-CREATION

          Diff: https://reviews.apache.org/r/1043/diff

          Testing

          -------

          test added, passing hudson qa bot.

          Thanks,

          Patrick

          Show
          jiraposter@reviews.apache.org added a comment - On 2011-07-07 23:40:32, Patrick Hunt wrote: > The documentation (src/docs) need to be updated - specifically the cleanup section in the admin guide. > > Have you considered hooking this into JMX or the 4letterwords? It would be nice for operators to get basic information. In JMX they could also control the settings... consider for a follow-on JIRA? Laxman Ch wrote: Fixed the documentation part. @Pat: I guess I'm not clear about how "hooking into JMX or 4 letter-words" will be helpful. Can you please explain this idea? I can takeup this task in separate JIRA. both JMX and 4letterwords provide insight to the operator into the runtime status of the system http://zookeeper.apache.org/doc/r3.3.3/zookeeperAdmin.html#sc_zkCommands http://zookeeper.apache.org/doc/r3.3.3/zookeeperJMX.html what I was suggesting is that you could allow such insight into the cleanup task - for example is the process running, when the last time it ran, a history of the files cleaned up and when, stop the task, restart, etc... Patrick ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/1043/#review998 ----------------------------------------------------------- On 2011-07-07 23:10:13, Patrick Hunt wrote: ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/1043/ ----------------------------------------------------------- (Updated 2011-07-07 23:10:13) Review request for zookeeper, Patrick Hunt, Benjamin Reed, and Mahadev Konar. Summary ------- I like to have ZK itself manage the amount of snapshots and logs kept, instead of relying on the PurgeTxnLog utility. This addresses bug ZOOKEEPER-1107 . https://issues.apache.org/jira/browse/ZOOKEEPER-1107 Diffs ----- ./conf/zoo_sample.cfg 1141901 ./src/java/main/org/apache/zookeeper/ZooKeeperPurger.java PRE-CREATION ./src/java/main/org/apache/zookeeper/server/quorum/QuorumPeerConfig.java 1141901 ./src/java/test/org/apache/zookeeper/ZooKeeperPurgeTest.java PRE-CREATION Diff: https://reviews.apache.org/r/1043/diff Testing ------- test added, passing hudson qa bot. Thanks, Patrick
          Hide
          jiraposter@reviews.apache.org added a comment -

          -----------------------------------------------------------
          This is an automatically generated e-mail. To reply, visit:
          https://reviews.apache.org/r/1043/
          -----------------------------------------------------------

          (Updated 2011-07-19 21:04:20.729172)

          Review request for zookeeper, Patrick Hunt, Benjamin Reed, and Mahadev Konar.

          Changes
          -------

          Updated with ZOOKEEPER-1107.4.patch

          Summary
          -------

          I like to have ZK itself manage the amount of snapshots and logs kept, instead of relying on the PurgeTxnLog utility.

          This addresses bug ZOOKEEPER-1107.
          https://issues.apache.org/jira/browse/ZOOKEEPER-1107

          Diffs (updated)


          ./conf/zoo_sample.cfg 1146568
          ./src/docs/src/documentation/content/xdocs/zookeeperAdmin.xml 1146568
          ./src/java/main/org/apache/zookeeper/server/DatadirCleanupManager.java PRE-CREATION
          ./src/java/main/org/apache/zookeeper/server/quorum/QuorumPeerConfig.java 1146568
          ./src/java/main/org/apache/zookeeper/server/quorum/QuorumPeerMain.java 1146568
          ./src/java/test/org/apache/zookeeper/server/DatadirCleanupManagerTest.java PRE-CREATION

          Diff: https://reviews.apache.org/r/1043/diff

          Testing
          -------

          test added, passing hudson qa bot.

          Thanks,

          Patrick

          Show
          jiraposter@reviews.apache.org added a comment - ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/1043/ ----------------------------------------------------------- (Updated 2011-07-19 21:04:20.729172) Review request for zookeeper, Patrick Hunt, Benjamin Reed, and Mahadev Konar. Changes ------- Updated with ZOOKEEPER-1107 .4.patch Summary ------- I like to have ZK itself manage the amount of snapshots and logs kept, instead of relying on the PurgeTxnLog utility. This addresses bug ZOOKEEPER-1107 . https://issues.apache.org/jira/browse/ZOOKEEPER-1107 Diffs (updated) ./conf/zoo_sample.cfg 1146568 ./src/docs/src/documentation/content/xdocs/zookeeperAdmin.xml 1146568 ./src/java/main/org/apache/zookeeper/server/DatadirCleanupManager.java PRE-CREATION ./src/java/main/org/apache/zookeeper/server/quorum/QuorumPeerConfig.java 1146568 ./src/java/main/org/apache/zookeeper/server/quorum/QuorumPeerMain.java 1146568 ./src/java/test/org/apache/zookeeper/server/DatadirCleanupManagerTest.java PRE-CREATION Diff: https://reviews.apache.org/r/1043/diff Testing ------- test added, passing hudson qa bot. Thanks, Patrick
          Hide
          jiraposter@reviews.apache.org added a comment -

          -----------------------------------------------------------
          This is an automatically generated e-mail. To reply, visit:
          https://reviews.apache.org/r/1043/#review1116
          -----------------------------------------------------------

          This is looking pretty close!

          ./conf/zoo_sample.cfg
          <https://reviews.apache.org/r/1043/#comment2277>

          I think something like

          datadircleanupmanager.snapRetainCount
          and
          datadircleanupmanager.purgeInterval

          would be better here – less ambiguous

          (in general we need to cleanup our configuration naming/handling at some point)

          ./src/docs/src/documentation/content/xdocs/zookeeperAdmin.xml
          <https://reviews.apache.org/r/1043/#comment2281>

          mention that this is "new in 3.4.0" – see some of the other parameters such as "clientPortAddress" for an example of how to do this.

          ./src/docs/src/documentation/content/xdocs/zookeeperAdmin.xml
          <https://reviews.apache.org/r/1043/#comment2278>

          something like the following?

          the <..>snapretaincount<..> most recent snapshots ...

          ./src/docs/src/documentation/content/xdocs/zookeeperAdmin.xml
          <https://reviews.apache.org/r/1043/#comment2279>

          replace "purges" with "deletes"?

          ./src/docs/src/documentation/content/xdocs/zookeeperAdmin.xml
          <https://reviews.apache.org/r/1043/#comment2280>

          do you mean something like:

          "Default is 3. Minimum value is 3."

          I think this would be a bit more obvious. Might also be good if we print a warning if user sets to less than 3 (and specify we are using 3)

          ./src/java/main/org/apache/zookeeper/server/DatadirCleanupManager.java
          <https://reviews.apache.org/r/1043/#comment2282>

          can we add a class javadoc describing this class?

          ./src/java/main/org/apache/zookeeper/server/DatadirCleanupManager.java
          <https://reviews.apache.org/r/1043/#comment2283>

          should be "DataDirCleanupManager.class" not nioservercnxn.class.

          ./src/java/main/org/apache/zookeeper/server/DatadirCleanupManager.java
          <https://reviews.apache.org/r/1043/#comment2285>

          move this to QPC, check it there, print a warning if user sets below this, and set the config field to this MIN.

          ./src/java/main/org/apache/zookeeper/server/DatadirCleanupManager.java
          <https://reviews.apache.org/r/1043/#comment2286>

          if we set this in the constructor we can define these as final.

          ./src/java/main/org/apache/zookeeper/server/DatadirCleanupManager.java
          <https://reviews.apache.org/r/1043/#comment2284>

          rather than add a dependency on QPC, could we pass these 4 parameters into a constructor for this class? Notice how this also makes your tests easier (no need to meddle with config).

          shouldn't we check the state before starting?

          ./src/java/main/org/apache/zookeeper/server/DatadirCleanupManager.java
          <https://reviews.apache.org/r/1043/#comment2287>

          move this to QPC

          ./src/java/main/org/apache/zookeeper/server/DatadirCleanupManager.java
          <https://reviews.apache.org/r/1043/#comment2288>

          I'd move this to the constructor of this class.

          ./src/java/main/org/apache/zookeeper/server/DatadirCleanupManager.java
          <https://reviews.apache.org/r/1043/#comment2289>

          nit - formatting is a bit off, typ

          if (foo < bar)

          { ... }

          ./src/java/main/org/apache/zookeeper/server/DatadirCleanupManager.java
          <https://reviews.apache.org/r/1043/#comment2292>

          final

          • Patrick

          On 2011-07-19 21:04:20, Patrick Hunt wrote:

          -----------------------------------------------------------

          This is an automatically generated e-mail. To reply, visit:

          https://reviews.apache.org/r/1043/

          -----------------------------------------------------------

          (Updated 2011-07-19 21:04:20)

          Review request for zookeeper, Patrick Hunt, Benjamin Reed, and Mahadev Konar.

          Summary

          -------

          I like to have ZK itself manage the amount of snapshots and logs kept, instead of relying on the PurgeTxnLog utility.

          This addresses bug ZOOKEEPER-1107.

          https://issues.apache.org/jira/browse/ZOOKEEPER-1107

          Diffs

          -----

          ./conf/zoo_sample.cfg 1146568

          ./src/docs/src/documentation/content/xdocs/zookeeperAdmin.xml 1146568

          ./src/java/main/org/apache/zookeeper/server/DatadirCleanupManager.java PRE-CREATION

          ./src/java/main/org/apache/zookeeper/server/quorum/QuorumPeerConfig.java 1146568

          ./src/java/main/org/apache/zookeeper/server/quorum/QuorumPeerMain.java 1146568

          ./src/java/test/org/apache/zookeeper/server/DatadirCleanupManagerTest.java PRE-CREATION

          Diff: https://reviews.apache.org/r/1043/diff

          Testing

          -------

          test added, passing hudson qa bot.

          Thanks,

          Patrick

          Show
          jiraposter@reviews.apache.org added a comment - ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/1043/#review1116 ----------------------------------------------------------- This is looking pretty close! ./conf/zoo_sample.cfg < https://reviews.apache.org/r/1043/#comment2277 > I think something like datadircleanupmanager.snapRetainCount and datadircleanupmanager.purgeInterval would be better here – less ambiguous (in general we need to cleanup our configuration naming/handling at some point) ./src/docs/src/documentation/content/xdocs/zookeeperAdmin.xml < https://reviews.apache.org/r/1043/#comment2281 > mention that this is "new in 3.4.0" – see some of the other parameters such as "clientPortAddress" for an example of how to do this. ./src/docs/src/documentation/content/xdocs/zookeeperAdmin.xml < https://reviews.apache.org/r/1043/#comment2278 > something like the following? the <..>snapretaincount<..> most recent snapshots ... ./src/docs/src/documentation/content/xdocs/zookeeperAdmin.xml < https://reviews.apache.org/r/1043/#comment2279 > replace "purges" with "deletes"? ./src/docs/src/documentation/content/xdocs/zookeeperAdmin.xml < https://reviews.apache.org/r/1043/#comment2280 > do you mean something like: "Default is 3. Minimum value is 3." I think this would be a bit more obvious. Might also be good if we print a warning if user sets to less than 3 (and specify we are using 3) ./src/java/main/org/apache/zookeeper/server/DatadirCleanupManager.java < https://reviews.apache.org/r/1043/#comment2282 > can we add a class javadoc describing this class? ./src/java/main/org/apache/zookeeper/server/DatadirCleanupManager.java < https://reviews.apache.org/r/1043/#comment2283 > should be "DataDirCleanupManager.class" not nioservercnxn.class. ./src/java/main/org/apache/zookeeper/server/DatadirCleanupManager.java < https://reviews.apache.org/r/1043/#comment2285 > move this to QPC, check it there, print a warning if user sets below this, and set the config field to this MIN. ./src/java/main/org/apache/zookeeper/server/DatadirCleanupManager.java < https://reviews.apache.org/r/1043/#comment2286 > if we set this in the constructor we can define these as final. ./src/java/main/org/apache/zookeeper/server/DatadirCleanupManager.java < https://reviews.apache.org/r/1043/#comment2284 > rather than add a dependency on QPC, could we pass these 4 parameters into a constructor for this class? Notice how this also makes your tests easier (no need to meddle with config). shouldn't we check the state before starting? ./src/java/main/org/apache/zookeeper/server/DatadirCleanupManager.java < https://reviews.apache.org/r/1043/#comment2287 > move this to QPC ./src/java/main/org/apache/zookeeper/server/DatadirCleanupManager.java < https://reviews.apache.org/r/1043/#comment2288 > I'd move this to the constructor of this class. ./src/java/main/org/apache/zookeeper/server/DatadirCleanupManager.java < https://reviews.apache.org/r/1043/#comment2289 > nit - formatting is a bit off, typ if (foo < bar) { ... } ./src/java/main/org/apache/zookeeper/server/DatadirCleanupManager.java < https://reviews.apache.org/r/1043/#comment2292 > final Patrick On 2011-07-19 21:04:20, Patrick Hunt wrote: ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/1043/ ----------------------------------------------------------- (Updated 2011-07-19 21:04:20) Review request for zookeeper, Patrick Hunt, Benjamin Reed, and Mahadev Konar. Summary ------- I like to have ZK itself manage the amount of snapshots and logs kept, instead of relying on the PurgeTxnLog utility. This addresses bug ZOOKEEPER-1107 . https://issues.apache.org/jira/browse/ZOOKEEPER-1107 Diffs ----- ./conf/zoo_sample.cfg 1146568 ./src/docs/src/documentation/content/xdocs/zookeeperAdmin.xml 1146568 ./src/java/main/org/apache/zookeeper/server/DatadirCleanupManager.java PRE-CREATION ./src/java/main/org/apache/zookeeper/server/quorum/QuorumPeerConfig.java 1146568 ./src/java/main/org/apache/zookeeper/server/quorum/QuorumPeerMain.java 1146568 ./src/java/test/org/apache/zookeeper/server/DatadirCleanupManagerTest.java PRE-CREATION Diff: https://reviews.apache.org/r/1043/diff Testing ------- test added, passing hudson qa bot. Thanks, Patrick
          Hide
          Patrick Hunt added a comment -

          I've provided some additional f/b through RB.

          Show
          Patrick Hunt added a comment - I've provided some additional f/b through RB.
          Hide
          Laxman added a comment -

          Thanks for your thorough review Pat.

          datadircleanupmanager.snapRetainCount
          and
          datadircleanupmanager.purgeInterval

          would be better here – less ambiguous

          (in general we need to cleanup our configuration naming/handling at some point)

          In my opinion, prefixing with "datadircleanupmanager" makes the configurations too long. How about naming this feature as "autopurge" prefixing with feature rather than class name like autopurge.snapRetainCount and autopurge.purgeInterval.

          Show
          Laxman added a comment - Thanks for your thorough review Pat. datadircleanupmanager.snapRetainCount and datadircleanupmanager.purgeInterval would be better here – less ambiguous (in general we need to cleanup our configuration naming/handling at some point) In my opinion, prefixing with "datadircleanupmanager" makes the configurations too long. How about naming this feature as "autopurge" prefixing with feature rather than class name like autopurge.snapRetainCount and autopurge.purgeInterval.
          Hide
          Laxman added a comment -

          Pat, any suggestion on this or shall I submit a patch as per my comments?

          Show
          Laxman added a comment - Pat, any suggestion on this or shall I submit a patch as per my comments?
          Hide
          Patrick Hunt added a comment -

          Yes, I like the "autopurge" prefix much better, good idea!

          Please do update the patch/RB and I'll take a look.

          Show
          Patrick Hunt added a comment - Yes, I like the "autopurge" prefix much better, good idea! Please do update the patch/RB and I'll take a look.
          Hide
          Laxman added a comment -

          Updated the patch based on review comments.

          Show
          Laxman added a comment - Updated the patch based on review comments.
          Hide
          Laxman added a comment -

          uploading patch again to get tested by qa bot.

          Show
          Laxman added a comment - uploading patch again to get tested by qa bot.
          Hide
          Hadoop QA added a comment -

          +1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12490409/ZOOKEEPER-1107.5.patch
          against trunk revision 1157698.

          +1 @author. The patch does not contain any @author tags.

          +1 tests included. The patch appears to include 3 new or modified tests.

          +1 javadoc. The javadoc tool did not generate any warning messages.

          +1 javac. The applied patch does not increase the total number of javac compiler warnings.

          +1 findbugs. The patch does not introduce any new Findbugs (version 1.3.9) warnings.

          +1 release audit. The applied patch does not increase the total number of release audit warnings.

          +1 core tests. The patch passed core unit tests.

          +1 contrib tests. The patch passed contrib unit tests.

          Test results: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/459//testReport/
          Findbugs warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/459//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
          Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/459//console

          This message is automatically generated.

          Show
          Hadoop QA added a comment - +1 overall. Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12490409/ZOOKEEPER-1107.5.patch against trunk revision 1157698. +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 3 new or modified tests. +1 javadoc. The javadoc tool did not generate any warning messages. +1 javac. The applied patch does not increase the total number of javac compiler warnings. +1 findbugs. The patch does not introduce any new Findbugs (version 1.3.9) warnings. +1 release audit. The applied patch does not increase the total number of release audit warnings. +1 core tests. The patch passed core unit tests. +1 contrib tests. The patch passed contrib unit tests. Test results: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/459//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/459//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/459//console This message is automatically generated.
          Hide
          jiraposter@reviews.apache.org added a comment -

          -----------------------------------------------------------
          This is an automatically generated e-mail. To reply, visit:
          https://reviews.apache.org/r/1043/
          -----------------------------------------------------------

          (Updated 2011-09-01 16:40:46.185217)

          Review request for zookeeper, Patrick Hunt, Benjamin Reed, and Mahadev Konar.

          Changes
          -------

          updated to 1107.5 patch

          Summary
          -------

          I like to have ZK itself manage the amount of snapshots and logs kept, instead of relying on the PurgeTxnLog utility.

          This addresses bug ZOOKEEPER-1107.
          https://issues.apache.org/jira/browse/ZOOKEEPER-1107

          Diffs (updated)


          ./src/java/main/org/apache/zookeeper/server/quorum/QuorumPeerMain.java 1149082
          ./src/java/main/org/apache/zookeeper/server/quorum/QuorumPeerConfig.java 1149082
          ./src/java/main/org/apache/zookeeper/server/DatadirCleanupManager.java PRE-CREATION
          ./conf/zoo_sample.cfg 1149082
          ./src/docs/src/documentation/content/xdocs/zookeeperAdmin.xml 1149082
          ./src/java/test/org/apache/zookeeper/server/DatadirCleanupManagerTest.java PRE-CREATION

          Diff: https://reviews.apache.org/r/1043/diff

          Testing
          -------

          test added, passing hudson qa bot.

          Thanks,

          Patrick

          Show
          jiraposter@reviews.apache.org added a comment - ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/1043/ ----------------------------------------------------------- (Updated 2011-09-01 16:40:46.185217) Review request for zookeeper, Patrick Hunt, Benjamin Reed, and Mahadev Konar. Changes ------- updated to 1107.5 patch Summary ------- I like to have ZK itself manage the amount of snapshots and logs kept, instead of relying on the PurgeTxnLog utility. This addresses bug ZOOKEEPER-1107 . https://issues.apache.org/jira/browse/ZOOKEEPER-1107 Diffs (updated) ./src/java/main/org/apache/zookeeper/server/quorum/QuorumPeerMain.java 1149082 ./src/java/main/org/apache/zookeeper/server/quorum/QuorumPeerConfig.java 1149082 ./src/java/main/org/apache/zookeeper/server/DatadirCleanupManager.java PRE-CREATION ./conf/zoo_sample.cfg 1149082 ./src/docs/src/documentation/content/xdocs/zookeeperAdmin.xml 1149082 ./src/java/test/org/apache/zookeeper/server/DatadirCleanupManagerTest.java PRE-CREATION Diff: https://reviews.apache.org/r/1043/diff Testing ------- test added, passing hudson qa bot. Thanks, Patrick
          Hide
          Hadoop QA added a comment -

          +1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12490409/ZOOKEEPER-1107.5.patch
          against trunk revision 1163106.

          +1 @author. The patch does not contain any @author tags.

          +1 tests included. The patch appears to include 3 new or modified tests.

          +1 javadoc. The javadoc tool did not generate any warning messages.

          +1 javac. The applied patch does not increase the total number of javac compiler warnings.

          +1 findbugs. The patch does not introduce any new Findbugs (version 1.3.9) warnings.

          +1 release audit. The applied patch does not increase the total number of release audit warnings.

          +1 core tests. The patch passed core unit tests.

          +1 contrib tests. The patch passed contrib unit tests.

          Test results: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/484//testReport/
          Findbugs warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/484//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
          Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/484//console

          This message is automatically generated.

          Show
          Hadoop QA added a comment - +1 overall. Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12490409/ZOOKEEPER-1107.5.patch against trunk revision 1163106. +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 3 new or modified tests. +1 javadoc. The javadoc tool did not generate any warning messages. +1 javac. The applied patch does not increase the total number of javac compiler warnings. +1 findbugs. The patch does not introduce any new Findbugs (version 1.3.9) warnings. +1 release audit. The applied patch does not increase the total number of release audit warnings. +1 core tests. The patch passed core unit tests. +1 contrib tests. The patch passed contrib unit tests. Test results: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/484//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/484//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/484//console This message is automatically generated.
          Hide
          Patrick Hunt added a comment -

          two small updates - one to the conf file and one to the admin guide.

          Show
          Patrick Hunt added a comment - two small updates - one to the conf file and one to the admin guide.
          Hide
          Patrick Hunt added a comment -

          @laxman - I made two small updates – the patch now looks good to me and I feel is ready to commit.

          However one thing I realized during this last review - it might be the case that people would want finer grain control on the frequency of purge (say they are getting large numbers of writes/second). So really having the period specified in minutes rather than hours is what I'm thinking.

          Does this make sense to you? Would you mind updating the patch for this? (my reading of TimerTask indicates that only a single purge operation would be running at any time, ie they will not run concurrently if I set the interval to be 1 minute and it takes more than a minute to purge, can you confirm?)

          Show
          Patrick Hunt added a comment - @laxman - I made two small updates – the patch now looks good to me and I feel is ready to commit. However one thing I realized during this last review - it might be the case that people would want finer grain control on the frequency of purge (say they are getting large numbers of writes/second). So really having the period specified in minutes rather than hours is what I'm thinking. Does this make sense to you? Would you mind updating the patch for this? (my reading of TimerTask indicates that only a single purge operation would be running at any time, ie they will not run concurrently if I set the interval to be 1 minute and it takes more than a minute to purge, can you confirm?)
          Hide
          Laxman added a comment -

          Thanks very much for your review and update Pat.

          having the period specified in minutes rather than hours

          1) In our case we have gone for this approach as disk is running out of space after long run say 30 days and even longer. Initially we thought of providing purge interval in days. But, considering the testability and your point of finer level of granularity only we have gone for hours.

          2) Say, for 3 days interval, user would prefer to configure 72 (hours) rather 4320 (minutes). Just trying to balance usability and granularity.

          Please let me know your thoughts on this. If you feel we need more granularity for some other usecase, surely I can update the patch.

          (my reading of TimerTask indicates that only a single purge operation would be running at any time, ie they will not run concurrently if I set the interval to be 1 minute and it takes more than a minute to purge, can you confirm?)

          Yes, verified the behavior of Java Timer. Timer will be running in one single thread and no two invocations will be running concurrently. Verified the same through debug points.

          Show
          Laxman added a comment - Thanks very much for your review and update Pat. having the period specified in minutes rather than hours 1) In our case we have gone for this approach as disk is running out of space after long run say 30 days and even longer. Initially we thought of providing purge interval in days. But, considering the testability and your point of finer level of granularity only we have gone for hours. 2) Say, for 3 days interval, user would prefer to configure 72 (hours) rather 4320 (minutes). Just trying to balance usability and granularity. Please let me know your thoughts on this. If you feel we need more granularity for some other usecase, surely I can update the patch. (my reading of TimerTask indicates that only a single purge operation would be running at any time, ie they will not run concurrently if I set the interval to be 1 minute and it takes more than a minute to purge, can you confirm?) Yes, verified the behavior of Java Timer. Timer will be running in one single thread and no two invocations will be running concurrently. Verified the same through debug points.
          Hide
          Patrick Hunt added a comment -

          @laxman sounds reasonable to me. If no one else objects I'll commit this.

          Show
          Patrick Hunt added a comment - @laxman sounds reasonable to me. If no one else objects I'll commit this.
          Hide
          Patrick Hunt added a comment -

          Thanks Laxman! Committed to 3.4.0

          Show
          Patrick Hunt added a comment - Thanks Laxman! Committed to 3.4.0
          Hide
          Hudson added a comment -

          Integrated in ZooKeeper-trunk-jdk7 #14 (See https://builds.apache.org/job/ZooKeeper-trunk-jdk7/14/)
          ZOOKEEPER-1107. automating log and snapshot cleaning (Laxman via phunt)

          phunt : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1164708
          Files :

          • /zookeeper/trunk/CHANGES.txt
          • /zookeeper/trunk/conf/zoo_sample.cfg
          • /zookeeper/trunk/src/docs/src/documentation/content/xdocs/zookeeperAdmin.xml
          • /zookeeper/trunk/src/java/main/org/apache/zookeeper/server/DatadirCleanupManager.java
          • /zookeeper/trunk/src/java/main/org/apache/zookeeper/server/quorum/QuorumPeerConfig.java
          • /zookeeper/trunk/src/java/main/org/apache/zookeeper/server/quorum/QuorumPeerMain.java
          • /zookeeper/trunk/src/java/test/org/apache/zookeeper/server/DatadirCleanupManagerTest.java
          Show
          Hudson added a comment - Integrated in ZooKeeper-trunk-jdk7 #14 (See https://builds.apache.org/job/ZooKeeper-trunk-jdk7/14/ ) ZOOKEEPER-1107 . automating log and snapshot cleaning (Laxman via phunt) phunt : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1164708 Files : /zookeeper/trunk/CHANGES.txt /zookeeper/trunk/conf/zoo_sample.cfg /zookeeper/trunk/src/docs/src/documentation/content/xdocs/zookeeperAdmin.xml /zookeeper/trunk/src/java/main/org/apache/zookeeper/server/DatadirCleanupManager.java /zookeeper/trunk/src/java/main/org/apache/zookeeper/server/quorum/QuorumPeerConfig.java /zookeeper/trunk/src/java/main/org/apache/zookeeper/server/quorum/QuorumPeerMain.java /zookeeper/trunk/src/java/test/org/apache/zookeeper/server/DatadirCleanupManagerTest.java
          Hide
          Hudson added a comment -

          Integrated in ZooKeeper-trunk #1293 (See https://builds.apache.org/job/ZooKeeper-trunk/1293/)
          ZOOKEEPER-1107. automating log and snapshot cleaning (Laxman via phunt)

          phunt : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1164708
          Files :

          • /zookeeper/trunk/CHANGES.txt
          • /zookeeper/trunk/conf/zoo_sample.cfg
          • /zookeeper/trunk/src/docs/src/documentation/content/xdocs/zookeeperAdmin.xml
          • /zookeeper/trunk/src/java/main/org/apache/zookeeper/server/DatadirCleanupManager.java
          • /zookeeper/trunk/src/java/main/org/apache/zookeeper/server/quorum/QuorumPeerConfig.java
          • /zookeeper/trunk/src/java/main/org/apache/zookeeper/server/quorum/QuorumPeerMain.java
          • /zookeeper/trunk/src/java/test/org/apache/zookeeper/server/DatadirCleanupManagerTest.java
          Show
          Hudson added a comment - Integrated in ZooKeeper-trunk #1293 (See https://builds.apache.org/job/ZooKeeper-trunk/1293/ ) ZOOKEEPER-1107 . automating log and snapshot cleaning (Laxman via phunt) phunt : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1164708 Files : /zookeeper/trunk/CHANGES.txt /zookeeper/trunk/conf/zoo_sample.cfg /zookeeper/trunk/src/docs/src/documentation/content/xdocs/zookeeperAdmin.xml /zookeeper/trunk/src/java/main/org/apache/zookeeper/server/DatadirCleanupManager.java /zookeeper/trunk/src/java/main/org/apache/zookeeper/server/quorum/QuorumPeerConfig.java /zookeeper/trunk/src/java/main/org/apache/zookeeper/server/quorum/QuorumPeerMain.java /zookeeper/trunk/src/java/test/org/apache/zookeeper/server/DatadirCleanupManagerTest.java

            People

            • Assignee:
              Laxman
              Reporter:
              Jun Rao
            • Votes:
              3 Vote for this issue
              Watchers:
              4 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development