[jira] Commented: (ZOOKEEPER-872) Small fixes to PurgeTxnLog

["Patrick Hunt (JIRA)" <ji...@apache.org>]

    [ https://issues.apache.org/jira/browse/ZOOKEEPER-872?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12927082#action_12927082 ] 

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 3");
+        if (num < 2) {
+            throw new IllegalArgumentException("count should be greater than 1");
         }

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

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