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