Re: Review Request: automating log and snapshot cleaning
--- 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
Re: Review Request: automating log and snapshot cleaning
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
Re: Review Request: automating log and snapshot cleaning
---
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
Re: Review Request: automating log and snapshot cleaning
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:
Re: Review Request: automating log and snapshot cleaning
--- 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
Re: Review Request: automating log and snapshot cleaning
--- 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
Re: Review Request: automating log and snapshot cleaning
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
Re: Review Request: automating log and snapshot cleaning
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
