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

getQueue(String queue) in JobTracker would return NPE for invalid queue name

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 0.21.0
    • Component/s: None
    • Labels:
      None
    • Hadoop Flags:
      Reviewed
    1. MAPREDUCE-1075-7.patch
      5 kB
      V.V.Chaitanya Krishna
    2. MAPREDUCE-1075-6.patch
      5 kB
      V.V.Chaitanya Krishna
    3. MAPREDUCE-1075-5.patch
      5 kB
      V.V.Chaitanya Krishna
    4. MAPREDUCE-1075-4.patch
      5 kB
      V.V.Chaitanya Krishna
    5. MAPREDUCE-1075-3.patch
      4 kB
      V.V.Chaitanya Krishna
    6. MAPREDUCE-1075-2.patch
      3 kB
      V.V.Chaitanya Krishna
    7. MAPREDUCE-1075-1.patch
      3 kB
      V.V.Chaitanya Krishna

      Issue Links

        Activity

        Hide
        V.V.Chaitanya Krishna added a comment -

        The problem in MAPREDUCE-1082 is partly due to this issue as well. So, linking this issue to MAPREDUCE-1082

        Show
        V.V.Chaitanya Krishna added a comment - The problem in MAPREDUCE-1082 is partly due to this issue as well. So, linking this issue to MAPREDUCE-1082
        Hide
        V.V.Chaitanya Krishna added a comment -

        It looks like MAPREDUCE-1082 issue has some other reasons and has no relation to this issue. So, removing the link issue.

        Show
        V.V.Chaitanya Krishna added a comment - It looks like MAPREDUCE-1082 issue has some other reasons and has no relation to this issue. So, removing the link issue.
        Hide
        V.V.Chaitanya Krishna added a comment -

        Uploading patch. This requires patch to MAPREDUCE-28.

        Show
        V.V.Chaitanya Krishna added a comment - Uploading patch. This requires patch to MAPREDUCE-28 .
        Hide
        Sreekanth Ramakrishnan added a comment -

        Took a look at the patch.

        We should handle the IOException at the client end and print only the message on the CLI.

        Testcase directly accesses the JobTracker why don't we use Client api's to check the same.

        Show
        Sreekanth Ramakrishnan added a comment - Took a look at the patch. We should handle the IOException at the client end and print only the message on the CLI. Testcase directly accesses the JobTracker why don't we use Client api's to check the same.
        Hide
        V.V.Chaitanya Krishna added a comment -

        Uploading patch with the above mentioned comments implemented.

        Show
        V.V.Chaitanya Krishna added a comment - Uploading patch with the above mentioned comments implemented.
        Hide
        V.V.Chaitanya Krishna added a comment -

        Missed out the exception to be thrown on client side. Uploading new patch with this added.

        Show
        V.V.Chaitanya Krishna added a comment - Missed out the exception to be thrown on client side. Uploading new patch with this added.
        Hide
        Sreekanth Ramakrishnan added a comment -

        The modification in the Cluster.java is not required, we should just handle the exception in JobQueueClient. Also, expected exception of IOException might hide some other IOException other than what we expected so we should probably only check for exception messages and remove Expected exception

        Show
        Sreekanth Ramakrishnan added a comment - The modification in the Cluster.java is not required, we should just handle the exception in JobQueueClient . Also, expected exception of IOException might hide some other IOException other than what we expected so we should probably only check for exception messages and remove Expected exception
        Hide
        V.V.Chaitanya Krishna added a comment -

        Uploading patch with the above comments implemented.

        Show
        V.V.Chaitanya Krishna added a comment - Uploading patch with the above comments implemented.
        Hide
        V.V.Chaitanya Krishna added a comment -

        Had an offline discussion with Sreekanth. There were couple of comments to assert after getting an existing queue in TestJobQueueClient.testGetQueue() and checking for jobQueueInfo object being null after getQueueInfo() is called.

        Uploading patch with these changes done.

        Show
        V.V.Chaitanya Krishna added a comment - Had an offline discussion with Sreekanth. There were couple of comments to assert after getting an existing queue in TestJobQueueClient.testGetQueue() and checking for jobQueueInfo object being null after getQueueInfo() is called. Uploading patch with these changes done.
        Hide
        Sreekanth Ramakrishnan added a comment -

        The changes looks fine to me.

        +1 to patch.

        Show
        Sreekanth Ramakrishnan added a comment - The changes looks fine to me. +1 to patch.
        Hide
        V.V.Chaitanya Krishna added a comment -

        Running through hudson.

        Show
        V.V.Chaitanya Krishna added a comment - Running through hudson.
        Hide
        Hadoop QA added a comment -

        -1 overall. Here are the results of testing the latest attachment
        http://issues.apache.org/jira/secure/attachment/12425189/MAPREDUCE-1075-5.patch
        against trunk revision 884628.

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

        +1 tests included. The patch appears to include 3 new or modified tests.

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

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

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

        -1 contrib tests. The patch failed contrib unit tests.

        Test results: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h3.grid.sp2.yahoo.net/153/testReport/
        Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h3.grid.sp2.yahoo.net/153/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
        Checkstyle results: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h3.grid.sp2.yahoo.net/153/artifact/trunk/build/test/checkstyle-errors.html
        Console output: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h3.grid.sp2.yahoo.net/153/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/12425189/MAPREDUCE-1075-5.patch against trunk revision 884628. +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 3 new or modified tests. +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 warnings. +1 release audit. The applied patch does not increase the total number of release audit warnings. +1 core tests. The patch passed core unit tests. -1 contrib tests. The patch failed contrib unit tests. Test results: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h3.grid.sp2.yahoo.net/153/testReport/ Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h3.grid.sp2.yahoo.net/153/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Checkstyle results: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h3.grid.sp2.yahoo.net/153/artifact/trunk/build/test/checkstyle-errors.html Console output: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h3.grid.sp2.yahoo.net/153/console This message is automatically generated.
        Hide
        V.V.Chaitanya Krishna added a comment -

        -1 contrib tests. The patch failed contrib unit tests.

        The test failure is due to MAPREDUCE-1245. It is not related to this issue.

        Show
        V.V.Chaitanya Krishna added a comment - -1 contrib tests. The patch failed contrib unit tests. The test failure is due to MAPREDUCE-1245 . It is not related to this issue.
        Hide
        Hemanth Yamijala added a comment -

        I looked at this patch. I am a bit concerned that we interpret every IOException to mean the queue does not exist. This may not be true if there are IOExceptions due to other cases like network issues etc. Why can't we return null in relevant APIs and interpret that to mean queue does not exist ? This pattern is similar to what we do if a job with a given id doesn't exist. One issue I can think about this approach is that more checks may be necessary on the client. But I feel it is cleaner. Thoughts ?

        Show
        Hemanth Yamijala added a comment - I looked at this patch. I am a bit concerned that we interpret every IOException to mean the queue does not exist. This may not be true if there are IOExceptions due to other cases like network issues etc. Why can't we return null in relevant APIs and interpret that to mean queue does not exist ? This pattern is similar to what we do if a job with a given id doesn't exist. One issue I can think about this approach is that more checks may be necessary on the client. But I feel it is cleaner. Thoughts ?
        Hide
        V.V.Chaitanya Krishna added a comment -

        Uploading patch with the above comments implemented.

        Show
        V.V.Chaitanya Krishna added a comment - Uploading patch with the above comments implemented.
        Hide
        Vinod Kumar Vavilapalli added a comment -

        Eventually, I think we should do ourselves away with these special interpretations on the client-side and simply return the result on success and throw exceptions in every failure possible. Not too strongly biased though to do or not to do this for getQueue() API in this JIRA issue itself to deal with the case of non-existent queue.

        Show
        Vinod Kumar Vavilapalli added a comment - Eventually, I think we should do ourselves away with these special interpretations on the client-side and simply return the result on success and throw exceptions in every failure possible. Not too strongly biased though to do or not to do this for getQueue() API in this JIRA issue itself to deal with the case of non-existent queue.
        Hide
        Hemanth Yamijala added a comment -

        Vinod, your approach will work if we can throw specific remote exceptions. For e.g. we could have a QueueNotFoundException and throw that remotely. This is supported by reasonable RPC implementations. I am not aware if this is supported in Hadoop. Is it ?

        Show
        Hemanth Yamijala added a comment - Vinod, your approach will work if we can throw specific remote exceptions. For e.g. we could have a QueueNotFoundException and throw that remotely. This is supported by reasonable RPC implementations. I am not aware if this is supported in Hadoop. Is it ?
        Hide
        Hemanth Yamijala added a comment -

        In an offline discussion with Vinod, we concluded that there is no provision to marshal exceptions in Hadoop's RPC right now. Hence, we are deciding in favor of returning null in the queue APIs.

        With this context I looked at the new patch. One minor NIT is that I would suggest we test the API JobClient.getQueueInfo instead of Cluster.getQueue, as it covers more code path that's changed. Can you please make this change and run the patch through Hudson so I can commit once it passes ?

        Show
        Hemanth Yamijala added a comment - In an offline discussion with Vinod, we concluded that there is no provision to marshal exceptions in Hadoop's RPC right now. Hence, we are deciding in favor of returning null in the queue APIs. With this context I looked at the new patch. One minor NIT is that I would suggest we test the API JobClient.getQueueInfo instead of Cluster.getQueue, as it covers more code path that's changed. Can you please make this change and run the patch through Hudson so I can commit once it passes ?
        Hide
        V.V.Chaitanya Krishna added a comment -

        Uploading patch with the above comments implemented.

        Show
        V.V.Chaitanya Krishna added a comment - Uploading patch with the above comments implemented.
        Hide
        V.V.Chaitanya Krishna added a comment -

        Running through hudson.

        Show
        V.V.Chaitanya Krishna added a comment - Running through hudson.
        Hide
        Hadoop QA added a comment -

        +1 overall. Here are the results of testing the latest attachment
        http://issues.apache.org/jira/secure/attachment/12427099/MAPREDUCE-1075-7.patch
        against trunk revision 887135.

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

        +1 tests included. The patch appears to include 3 new or modified tests.

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

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

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

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

        Test results: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h6.grid.sp2.yahoo.net/295/testReport/
        Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h6.grid.sp2.yahoo.net/295/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
        Checkstyle results: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h6.grid.sp2.yahoo.net/295/artifact/trunk/build/test/checkstyle-errors.html
        Console output: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h6.grid.sp2.yahoo.net/295/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/12427099/MAPREDUCE-1075-7.patch against trunk revision 887135. +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 3 new or modified tests. +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 warnings. +1 release audit. The applied patch does not increase the total number of release audit warnings. +1 core tests. The patch passed core unit tests. +1 contrib tests. The patch passed contrib unit tests. Test results: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h6.grid.sp2.yahoo.net/295/testReport/ Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h6.grid.sp2.yahoo.net/295/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Checkstyle results: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h6.grid.sp2.yahoo.net/295/artifact/trunk/build/test/checkstyle-errors.html Console output: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h6.grid.sp2.yahoo.net/295/console This message is automatically generated.
        Hide
        Hemanth Yamijala added a comment -

        I just committed this to trunk and branch 0.21. Thanks, Chaitanya.

        Show
        Hemanth Yamijala added a comment - I just committed this to trunk and branch 0.21. Thanks, Chaitanya.
        Hide
        Hudson added a comment -

        Integrated in Hadoop-Mapreduce-trunk-Commit #147 (See http://hudson.zones.apache.org/hudson/job/Hadoop-Mapreduce-trunk-Commit/147/)
        . Fix JobTracker to not throw an NPE for a non-existent queue. Contributed by V.V.Chaitanya Krishna.

        Show
        Hudson added a comment - Integrated in Hadoop-Mapreduce-trunk-Commit #147 (See http://hudson.zones.apache.org/hudson/job/Hadoop-Mapreduce-trunk-Commit/147/ ) . Fix JobTracker to not throw an NPE for a non-existent queue. Contributed by V.V.Chaitanya Krishna.

          People

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

            Dates

            • Created:
              Updated:
              Resolved:

              Development