> 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 > >
