Review Request: automating log and snapshot cleaning

[Patrick Hunt]

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

Re: Review Request: automating log and snapshot cleaning

[Camille Fournier]

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

[Patrick Hunt]

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


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


should this be in zookeeper or zookeeper.server ?



./src/java/main/org/apache/zookeeper/ZooKeeperPurger.java


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



./src/java/main/org/apache/zookeeper/ZooKeeperPurger.java


enum?



./src/java/main/org/apache/zookeeper/ZooKeeperPurger.java


use TimeUnit



./src/java/main/org/apache/zookeeper/ZooKeeperPurger.java


perhaps this should be done in start?



./src/java/main/org/apache/zookeeper/ZooKeeperPurger.java


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


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



./src/java/main/org/apache/zookeeper/ZooKeeperPurger.java


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


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



./src/java/main/org/apache/zookeeper/ZooKeeperPurger.java


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



./src/java/main/org/apache/zookeeper/ZooKeeperPurger.java


Zookeeper should be referred to as "ZooKeeper"



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


specify explicit default, 3?



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


specify explicit default - e.g. 0.



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


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


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



./src/java/test/org/apache/zookeeper/ZooKeeperPurgeTest.java


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

[Patrick Hunt]

> 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

[Patrick Hunt]

> On 2011-07-07 23:40:32, Patrick Hunt wrote:
> > ./src/java/main/org/apache/zookeeper/ZooKeeperPurger.java, lines 74-77
> > 
> >
> > 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
> 
>

Re: Review Request: automating log and snapshot cleaning

[Laxman Ch]

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

Re: Review Request: automating log and snapshot cleaning

[Patrick Hunt]

> 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

[Patrick Hunt]

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

Re: Review Request: automating log and snapshot cleaning

[Patrick Hunt]

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


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


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


something like the following?

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



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


replace "purges" with "deletes"?



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


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


can we add a class javadoc describing this class?



./src/java/main/org/apache/zookeeper/server/DatadirCleanupManager.java


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



./src/java/main/org/apache/zookeeper/server/DatadirCleanupManager.java


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


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



./src/java/main/org/apache/zookeeper/server/DatadirCleanupManager.java


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


move this to QPC



./src/java/main/org/apache/zookeeper/server/DatadirCleanupManager.java


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



./src/java/main/org/apache/zookeeper/server/DatadirCleanupManager.java


nit - formatting is a bit off, typ

if (foo < bar) {
...
}




./src/java/main/org/apache/zookeeper/server/DatadirCleanupManager.java


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

[Patrick Hunt]

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