Hadoop Map/Reduce
  1. Hadoop Map/Reduce
  2. MAPREDUCE-1002

After MAPREDUCE-862, command line queue-list doesn't print any queues

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 0.21.0
    • Component/s: client
    • Labels:
      None
    • Hadoop Flags:
      Reviewed

      Description

      Web-ui correctly prints the queues, it is the command line that is not showing any queues.

      1. MAPREDUCE-1002.patch
        0.5 kB
        V.V.Chaitanya Krishna
      2. MAPREDUCE-1002-1.patch
        0.7 kB
        V.V.Chaitanya Krishna

        Activity

        Vinod Kumar Vavilapalli created issue -
        Hide
        Vinod Kumar Vavilapalli added a comment -

        The same problem is with queue -info <queue-name> too.

        Sreekanth and Chaitanya quickly debugged into this and found out the reason for this to be a missing flush of stream in the JobQueueClient.

        Show
        Vinod Kumar Vavilapalli added a comment - The same problem is with queue -info <queue-name> too. Sreekanth and Chaitanya quickly debugged into this and found out the reason for this to be a missing flush of stream in the JobQueueClient.
        Sreekanth Ramakrishnan made changes -
        Field Original Value New Value
        Assignee V.V.Chaitanya Krishna [ chaitk ]
        Hide
        V.V.Chaitanya Krishna added a comment -

        Uploading fix patch for this issue.

        Show
        V.V.Chaitanya Krishna added a comment - Uploading fix patch for this issue.
        V.V.Chaitanya Krishna made changes -
        Attachment MAPREDUCE-1002.patch [ 12419976 ]
        Hide
        Hemanth Yamijala added a comment -

        Hmm. Do we need a flush in the case where an invalid queue is given too ?

        Show
        Hemanth Yamijala added a comment - Hmm. Do we need a flush in the case where an invalid queue is given too ?
        Hide
        V.V.Chaitanya Krishna added a comment -

        flush seems to be required when queue is not found. Uploading patch with this case also handled.

        Show
        V.V.Chaitanya Krishna added a comment - flush seems to be required when queue is not found. Uploading patch with this case also handled.
        V.V.Chaitanya Krishna made changes -
        Attachment MAPREDUCE-1002-1.patch [ 12419981 ]
        Hide
        Hemanth Yamijala added a comment -

        Looks good to me. +1. I don't suppose we can write a unit test for this ? Can Vinod confirm that this fixes the problem, and we can start the test-patch and test runs ?

        Show
        Hemanth Yamijala added a comment - Looks good to me. +1. I don't suppose we can write a unit test for this ? Can Vinod confirm that this fixes the problem, and we can start the test-patch and test runs ?
        Hide
        Vinod Kumar Vavilapalli added a comment -

        I verified on a single node cluster that flush() call works. +1 for the fix.

        Show
        Vinod Kumar Vavilapalli added a comment - I verified on a single node cluster that flush() call works. +1 for the fix.
        Hide
        V.V.Chaitanya Krishna added a comment -

        ran test-patch locally. It gives +1.

        [exec] +1 overall.
        [exec]
        [exec] +1 @author. The patch does not contain any @author tags.
        [exec]
        [exec] -1 tests included. The patch doesn't appear to include any new or modified tests.
        [exec] Please justify why no new tests are needed for this patch.
        [exec] Also please list what manual steps were performed to verify this patch.
        [exec]
        [exec] +1 javadoc. The javadoc tool did not generate any warning messages.
        [exec]
        [exec] +1 javac. The applied patch does not increase the total number of javac compiler warnings.
        [exec]
        [exec] +1 findbugs. The patch does not introduce any new Findbugs warnings.
        [exec]
        [exec] +1 release audit. The applied patch does not increase the total number of release audit warnings.
        [exec]
        [exec]
        [exec]
        [exec]
        [exec] ======================================================================
        [exec] ======================================================================
        [exec] Finished build.
        [exec] ======================================================================
        [exec] ======================================================================

        Show
        V.V.Chaitanya Krishna added a comment - ran test-patch locally. It gives +1. [exec] +1 overall. [exec] [exec] +1 @author. The patch does not contain any @author tags. [exec] [exec] -1 tests included. The patch doesn't appear to include any new or modified tests. [exec] Please justify why no new tests are needed for this patch. [exec] Also please list what manual steps were performed to verify this patch. [exec] [exec] +1 javadoc. The javadoc tool did not generate any warning messages. [exec] [exec] +1 javac. The applied patch does not increase the total number of javac compiler warnings. [exec] [exec] +1 findbugs. The patch does not introduce any new Findbugs warnings. [exec] [exec] +1 release audit. The applied patch does not increase the total number of release audit warnings. [exec] [exec] [exec] [exec] [exec] ====================================================================== [exec] ====================================================================== [exec] Finished build. [exec] ====================================================================== [exec] ======================================================================
        Hide
        V.V.Chaitanya Krishna added a comment -

        the -1 for tests is beacuse there are no unit tests to this patch.

        Show
        V.V.Chaitanya Krishna added a comment - the -1 for tests is beacuse there are no unit tests to this patch.
        Hide
        Hemanth Yamijala added a comment -

        the -1 for tests is beacuse there are no unit tests to this patch.

        Given the fix is the addition of two writer.flush() calls that can't be verified more directly, and manual verification is done, I am fine with this.

        Show
        Hemanth Yamijala added a comment - the -1 for tests is beacuse there are no unit tests to this patch. Given the fix is the addition of two writer.flush() calls that can't be verified more directly, and manual verification is done, I am fine with this.
        Hide
        V.V.Chaitanya Krishna added a comment -

        ran ant test and it passed successfully

        Show
        V.V.Chaitanya Krishna added a comment - ran ant test and it passed successfully
        Hide
        Hemanth Yamijala added a comment -

        I just committed this. Thanks, Chaitanya !

        Show
        Hemanth Yamijala added a comment - I just committed this. Thanks, Chaitanya !
        Hemanth Yamijala made changes -
        Status Open [ 1 ] Resolved [ 5 ]
        Hadoop Flags [Reviewed]
        Resolution Fixed [ 1 ]
        Hide
        Nigel Daley added a comment -

        I don't understand why this was committed with no unit tests. Hemanth, Chaitanya, can you guarantee this won't break again in the future?

        We have cli tests in mapreduce. Would it not have been a simple addition to that?

        Show
        Nigel Daley added a comment - I don't understand why this was committed with no unit tests. Hemanth, Chaitanya, can you guarantee this won't break again in the future? We have cli tests in mapreduce. Would it not have been a simple addition to that?
        Hide
        Hemanth Yamijala added a comment -

        Nigel, we already have a test in MAPREDUCE-862 that validates output of a command like TestCLI does. The test captures the output and validates what's expected. However, the writer we use in the test case does not seem to require a flush to get the data. So, the test passes. However, when we tested on a real cluster, the flush was required with the System.out stream.

        Also, it was not a simple addition to TestCLI. IIRC, we needed to add the queue command type to the TestCLI class. And that requires a patch to common. I think that is undesirable. TestCLI should be refactored so projects can add commands without having to submit patches to common, an effort for which there was no time close to the feature freeze.

        Show
        Hemanth Yamijala added a comment - Nigel, we already have a test in MAPREDUCE-862 that validates output of a command like TestCLI does. The test captures the output and validates what's expected. However, the writer we use in the test case does not seem to require a flush to get the data. So, the test passes. However, when we tested on a real cluster, the flush was required with the System.out stream. Also, it was not a simple addition to TestCLI. IIRC, we needed to add the queue command type to the TestCLI class. And that requires a patch to common. I think that is undesirable. TestCLI should be refactored so projects can add commands without having to submit patches to common, an effort for which there was no time close to the feature freeze.
        Tom White made changes -
        Status Resolved [ 5 ] Closed [ 6 ]

          People

          • Assignee:
            V.V.Chaitanya Krishna
            Reporter:
            Vinod Kumar Vavilapalli
          • Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development