Patrick Hunt commented on ZOOKEEPER-872:

Hi Vishal, I noticed a couple issues.

This class is a command line utility. As such we are outputting to both the 
command line and to the log. The usage() in particular should go to std out so 
that the user will see it regardless of the log settings (fine if you want to 
output it to LOG as well, but I think this is unnecessary).

good catch on the error handling for this:

     public static void purge(File dataDir, File snapDir, int num) throws 
IOException {
-        if (num < 3) {
-            throw new IllegalArgumentException("count should be greater than 
+        if (num < 2) {
+            throw new IllegalArgumentException("count should be greater than 

However the number 3 was chosen to ensure that ppl don't shoot themselves in 
the foot (if the most recent logs get corrupted we'll fall back to the prior 
when attempting to recover). There really should be a comment to this effect 
(would be good to add). I don't know how Mahadev feels on this setting (min 3 
vs some other number) but he might have more insight as IIRC he implemented 
this originally.

this following is there to provide feedback to the user when running on command 

-            System.out.println("Removing file: "+
-                DateFormat.getDateTimeInstance().format(f.lastModified())+
-                "\t"+f.getPath());

again, regardless of logging setup.

Perhaps we should have a "-q" option that turns off the CLI logging and just 
logs to the log file? I know this has been an issue previously (stdout/err) 
given that cron will spitout emails by default containing stdout/err.

Also, is there a test for this? If you're up to it would be great to add.

> Small fixes to PurgeTxnLog 
> ---------------------------
>                 Key: ZOOKEEPER-872
>                 URL: https://issues.apache.org/jira/browse/ZOOKEEPER-872
>             Project: Zookeeper
>          Issue Type: Bug
>    Affects Versions: 3.3.1
>            Reporter: Vishal K
>            Assignee: Vishal K
>            Priority: Minor
>             Fix For: 3.4.0
>         Attachments: ZOOKEEPER-872
> PurgeTxnLog forces us to have at least 2 backups (by having count >= 3. Also, 
> it prints to stdout instead of using Logger.

This message is automatically generated by JIRA.
You can reply to this email to add a comment to the issue online.

Reply via email to