Hadoop HDFS
  1. Hadoop HDFS
  2. HDFS-1958

Format confirmation prompt should be more lenient of its input

    Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 0.22.0
    • Fix Version/s: 0.23.0
    • Component/s: namenode
    • Labels:
      None
    • Hadoop Flags:
      Reviewed

      Description

      As reported on the mailing list, the namenode format prompt only accepts 'Y'. We should also accept 'y' and 'yes' (non-case-sensitive).

      1. hdfs-1958.txt
        3 kB
        Todd Lipcon

        Activity

        Arun C Murthy made changes -
        Status Resolved [ 5 ] Closed [ 6 ]
        Hide
        Hudson added a comment -

        Integrated in Hadoop-Hdfs-trunk #673 (See https://builds.apache.org/hudson/job/Hadoop-Hdfs-trunk/673/)

        Show
        Hudson added a comment - Integrated in Hadoop-Hdfs-trunk #673 (See https://builds.apache.org/hudson/job/Hadoop-Hdfs-trunk/673/ )
        Hide
        Hudson added a comment -

        Integrated in Hadoop-Hdfs-trunk-Commit #677 (See https://builds.apache.org/hudson/job/Hadoop-Hdfs-trunk-Commit/677/)

        Show
        Hudson added a comment - Integrated in Hadoop-Hdfs-trunk-Commit #677 (See https://builds.apache.org/hudson/job/Hadoop-Hdfs-trunk-Commit/677/ )
        Hide
        Tsz Wo Nicholas Sze added a comment -

        > OK, another comparison: mke2fs doesn't ask for confirmation at all. I checked this across ext2, ext3, and ntfs.

        Not asking for confirmation and case insensitive are also different.

        Moreover, since the "-format" command is there for years, I wonder if there are some admins already taking advantages of the fact that 'y' won't format. For example, the 'yes' command outputs lower case y's.

        > Yes. But I already addressed it so no need to retroactively tag it as such (IMO the point of the newbie label is just to help new contributors find open JIRAs that might be easy to start with).

        Okay, I think you like to leave the easy issues for the new contributors.

        Show
        Tsz Wo Nicholas Sze added a comment - > OK, another comparison: mke2fs doesn't ask for confirmation at all. I checked this across ext2, ext3, and ntfs. Not asking for confirmation and case insensitive are also different. Moreover, since the "-format" command is there for years, I wonder if there are some admins already taking advantages of the fact that 'y' won't format. For example, the 'yes' command outputs lower case y's. > Yes. But I already addressed it so no need to retroactively tag it as such (IMO the point of the newbie label is just to help new contributors find open JIRAs that might be easy to start with). Okay, I think you like to leave the easy issues for the new contributors.
        Hide
        Todd Lipcon added a comment -

        However, it is not as convincing as suggested to change a feature affecting user behavior simply because someone has reported on the mailing list.

        Fair enough. I'll try to reproduce the reasoning from the mailing list in the future.

        Todd, do you agree that this is a "newbie" issue

        Yes. But I already addressed it so no need to retroactively tag it as such (IMO the point of the newbie label is just to help new contributors find open JIRAs that might be easy to start with).

        Show
        Todd Lipcon added a comment - However, it is not as convincing as suggested to change a feature affecting user behavior simply because someone has reported on the mailing list. Fair enough. I'll try to reproduce the reasoning from the mailing list in the future. Todd, do you agree that this is a "newbie" issue Yes. But I already addressed it so no need to retroactively tag it as such (IMO the point of the newbie label is just to help new contributors find open JIRAs that might be easy to start with).
        Hide
        Tsz Wo Nicholas Sze added a comment -

        Todd, do you agree that this is a "newbie" issue?

        Show
        Tsz Wo Nicholas Sze added a comment - Todd, do you agree that this is a "newbie" issue?
        Hide
        Tsz Wo Nicholas Sze added a comment -

        > Excuse me - I took Nicholas's question for a joke, to be honest, given it referenced high school students and didn't raise technical objections.

        It is a half joke.

        However, it is not as convincing as suggested to change a feature affecting user behavior simply because someone has reported on the mailing list.

        Show
        Tsz Wo Nicholas Sze added a comment - > Excuse me - I took Nicholas's question for a joke, to be honest, given it referenced high school students and didn't raise technical objections. It is a half joke. However, it is not as convincing as suggested to change a feature affecting user behavior simply because someone has reported on the mailing list.
        Hide
        Todd Lipcon added a comment -

        btw, for those who might be concerned about accidentally formatting the NN (perhaps your cat likes to jump on the 'y' key and then the enter key), you can also enable HDFS-718 to completely disallow it.

        Show
        Todd Lipcon added a comment - btw, for those who might be concerned about accidentally formatting the NN (perhaps your cat likes to jump on the 'y' key and then the enter key), you can also enable HDFS-718 to completely disallow it.
        Hide
        Todd Lipcon added a comment -

        particularly when an experienced commmitter is raising questions about it?

        Excuse me - I took Nicholas's question for a joke, to be honest, given it referenced high school students and didn't raise technical objections.

        fsck and format are different: it is okay to accidentally click "y" for fsck but not for format.

        OK, another comparison: mke2fs doesn't ask for confirmation at all. I checked this across ext2, ext3, and ntfs.

        Show
        Todd Lipcon added a comment - particularly when an experienced commmitter is raising questions about it? Excuse me - I took Nicholas's question for a joke, to be honest, given it referenced high school students and didn't raise technical objections. fsck and format are different: it is okay to accidentally click "y" for fsck but not for format. OK, another comparison: mke2fs doesn't ask for confirmation at all. I checked this across ext2, ext3, and ntfs.
        Hide
        Jakob Homan added a comment -

        Less than 24 hours between this issue being opened and committed, on non-critical issues, seems a little short. Perhaps the community should be given more of a chance to weigh in before committing things, particularly when an experienced commmitter is raising questions about it?

        Show
        Jakob Homan added a comment - Less than 24 hours between this issue being opened and committed, on non-critical issues, seems a little short. Perhaps the community should be given more of a chance to weigh in before committing things, particularly when an experienced commmitter is raising questions about it?
        Hide
        Tsz Wo Nicholas Sze added a comment -

        > well, fsck for example supports either 'y' or 'Y' for yes, and 'n' or 'N' for no.

        fsck and format are different: it is okay to accidentally click "y" for fsck but not for format.

        I was asking if this is a good feature in my previous comment. I tried to change a few years back but I heard one argument saying that the command was deliberately designed for preventing accidentally format.

        BTW, I think this should be classified as a "newbie" issue once we have decided to do it.

        Show
        Tsz Wo Nicholas Sze added a comment - > well, fsck for example supports either 'y' or 'Y' for yes, and 'n' or 'N' for no. fsck and format are different: it is okay to accidentally click "y" for fsck but not for format. I was asking if this is a good feature in my previous comment . I tried to change a few years back but I heard one argument saying that the command was deliberately designed for preventing accidentally format. BTW, I think this should be classified as a "newbie" issue once we have decided to do it.
        Todd Lipcon made changes -
        Status Patch Available [ 10002 ] Resolved [ 5 ]
        Hadoop Flags [Reviewed]
        Fix Version/s 0.23.0 [ 12315571 ]
        Fix Version/s 0.22.0 [ 12314241 ]
        Resolution Fixed [ 1 ]
        Hide
        Todd Lipcon added a comment -

        Thanks for review, Eli. I elected to only commit this to trunk since it's a new feature/improvement.

        Show
        Todd Lipcon added a comment - Thanks for review, Eli. I elected to only commit this to trunk since it's a new feature/improvement.
        Hide
        Eli Collins added a comment -

        +1 lgtm

        Show
        Eli Collins added a comment - +1 lgtm
        Hide
        Hadoop QA added a comment -

        -1 overall. Here are the results of testing the latest attachment
        http://issues.apache.org/jira/secure/attachment/12479702/hdfs-1958.txt
        against trunk revision 1124459.

        +1 @author. The patch does not contain any @author tags.

        -1 tests included. The patch doesn't appear to include any new or modified tests.
        Please justify why no new tests are needed for this patch.
        Also please list what manual steps were performed to verify this patch.

        +1 javadoc. The javadoc tool did not generate any warning messages.

        +1 javac. The applied patch does not increase the total number of javac compiler warnings.

        +1 findbugs. The patch does not introduce any new Findbugs (version 1.3.9) warnings.

        +1 release audit. The applied patch does not increase the total number of release audit warnings.

        -1 core tests. The patch failed these core unit tests:
        org.apache.hadoop.hdfs.TestDFSStorageStateRecovery
        org.apache.hadoop.hdfs.TestFileConcurrentReader
        org.apache.hadoop.tools.TestJMXGet

        +1 contrib tests. The patch passed contrib unit tests.

        +1 system test framework. The patch passed system test framework compile.

        Test results: https://builds.apache.org/hudson/job/PreCommit-HDFS-Build/575//testReport/
        Findbugs warnings: https://builds.apache.org/hudson/job/PreCommit-HDFS-Build/575//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
        Console output: https://builds.apache.org/hudson/job/PreCommit-HDFS-Build/575//console

        This message is automatically generated.

        Show
        Hadoop QA added a comment - -1 overall. Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12479702/hdfs-1958.txt against trunk revision 1124459. +1 @author. The patch does not contain any @author tags. -1 tests included. The patch doesn't appear to include any new or modified tests. Please justify why no new tests are needed for this patch. Also please list what manual steps were performed to verify this patch. +1 javadoc. The javadoc tool did not generate any warning messages. +1 javac. The applied patch does not increase the total number of javac compiler warnings. +1 findbugs. The patch does not introduce any new Findbugs (version 1.3.9) warnings. +1 release audit. The applied patch does not increase the total number of release audit warnings. -1 core tests. The patch failed these core unit tests: org.apache.hadoop.hdfs.TestDFSStorageStateRecovery org.apache.hadoop.hdfs.TestFileConcurrentReader org.apache.hadoop.tools.TestJMXGet +1 contrib tests. The patch passed contrib unit tests. +1 system test framework. The patch passed system test framework compile. Test results: https://builds.apache.org/hudson/job/PreCommit-HDFS-Build/575//testReport/ Findbugs warnings: https://builds.apache.org/hudson/job/PreCommit-HDFS-Build/575//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Console output: https://builds.apache.org/hudson/job/PreCommit-HDFS-Build/575//console This message is automatically generated.
        Hide
        Todd Lipcon added a comment -

        btw, I tested the attached patch manually by running "hadoop namenode -format" and trying the various possibilities. It's not possible to test automatically since it's by definition an interactive prompt.

        Show
        Todd Lipcon added a comment - btw, I tested the attached patch manually by running "hadoop namenode -format" and trying the various possibilities. It's not possible to test automatically since it's by definition an interactive prompt.
        Hide
        Todd Lipcon added a comment -

        Philip: please see HDFS-1636 for the issue about the empty directory case. It seems to be in-flight. Mind filing another one for the improvement to add a "-y" option to the format command?

        Show
        Todd Lipcon added a comment - Philip: please see HDFS-1636 for the issue about the empty directory case. It seems to be in-flight. Mind filing another one for the improvement to add a "-y" option to the format command?
        Todd Lipcon made changes -
        Status Open [ 1 ] Patch Available [ 10002 ]
        Hide
        Todd Lipcon added a comment -

        well, fsck for example supports either 'y' or 'Y' for yes, and 'n' or 'N' for no.

        fsck also does the following which we can't replicate in Java:

                tcgetattr (0, &termios);
                tmp = termios;
                tmp.c_lflag &= ~(ICANON | ECHO);
                tmp.c_cc[VMIN] = 1;
                tmp.c_cc[VTIME] = 0;
                tcsetattr (0, TCSANOW, &tmp);
        

        so that it can do an unbuffered read of a single keypress from stdin. Thus if you start to type "yes" or "no" it will see the "y" or "n" and act as expected.

        Since we can't do that, accepting "yes" or "no" seems like a good idea!

        [to satisfy Allen's inevitable gripe, I'm referring to the Linux-specific e2fsck behavior here]

        Show
        Todd Lipcon added a comment - well, fsck for example supports either 'y' or 'Y' for yes, and 'n' or 'N' for no. fsck also does the following which we can't replicate in Java: tcgetattr (0, &termios); tmp = termios; tmp.c_lflag &= ~(ICANON | ECHO); tmp.c_cc[VMIN] = 1; tmp.c_cc[VTIME] = 0; tcsetattr (0, TCSANOW, &tmp); so that it can do an unbuffered read of a single keypress from stdin. Thus if you start to type "yes" or "no" it will see the "y" or "n" and act as expected. Since we can't do that, accepting "yes" or "no" seems like a good idea! [to satisfy Allen's inevitable gripe, I'm referring to the Linux-specific e2fsck behavior here]
        Hide
        Philip Zeyliger added a comment -

        If we're editing this code, the fact that it can't be forced on the command line, and the fact that it treats an empty directory as one that already has a namenode image on it are also usability issues if you're scripting around namenode formatting.

        Show
        Philip Zeyliger added a comment - If we're editing this code, the fact that it can't be forced on the command line, and the fact that it treats an empty directory as one that already has a namenode image on it are also usability issues if you're scripting around namenode formatting.
        Todd Lipcon made changes -
        Field Original Value New Value
        Attachment hdfs-1958.txt [ 12479702 ]
        Hide
        Todd Lipcon added a comment -

        Attached patch makes the following change:

        • all confirmation prompts accept "y" or "yes", case-insensitive, as a positive response
        • "n" or "no" act as negative responses
        • anything else will repeat the prompt until a valid response is given
        Show
        Todd Lipcon added a comment - Attached patch makes the following change: all confirmation prompts accept "y" or "yes", case-insensitive, as a positive response "n" or "no" act as negative responses anything else will repeat the prompt until a valid response is given
        Hide
        Tsz Wo Nicholas Sze added a comment -

        Who reported it? A 20 year experience unix system admin, a high school student or someone else?

        Show
        Tsz Wo Nicholas Sze added a comment - Who reported it? A 20 year experience unix system admin, a high school student or someone else?
        Todd Lipcon created issue -

          People

          • Assignee:
            Todd Lipcon
            Reporter:
            Todd Lipcon
          • Votes:
            0 Vote for this issue
            Watchers:
            5 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development