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

Gridmix3 doesn't emit out proper mesage when user resolver is set and no user list is given

    Details

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

      Description

      Currently, Gridmix3 emits out the following mesage when user resolver is set and no user list is given.
      "Resource null ignored".
      This is not clear/meaningful to user.

      1. 1989.v1.3.patch
        13 kB
        Ravi Gummadi
      2. 1989.v1.2.patch
        14 kB
        Ravi Gummadi
      3. 1989.v1.1.patch
        12 kB
        Ravi Gummadi
      4. 1989.v1.patch
        12 kB
        Ravi Gummadi
      5. 1989.patch
        2 kB
        Ravi Gummadi

        Activity

        Hide
        Ravi Gummadi added a comment -

        If user list is not given, but user resolver is set to RoundRobinUserResolver, then IOException is thrown.
        For other user resolvers, we see "Resource null ignored" as a warning message.

        Show
        Ravi Gummadi added a comment - If user list is not given, but user resolver is set to RoundRobinUserResolver, then IOException is thrown. For other user resolvers, we see "Resource null ignored" as a warning message.
        Hide
        Ravi Gummadi added a comment -

        Attaching patch fixing the error message for different cases. Also modified the meesage to be displayed for the case of "RoundRobinUserResolver with empty list of users causing stack trace with IOException to be displayed".

        Show
        Ravi Gummadi added a comment - Attaching patch fixing the error message for different cases. Also modified the meesage to be displayed for the case of "RoundRobinUserResolver with empty list of users causing stack trace with IOException to be displayed".
        Hide
        Ravi Gummadi added a comment -

        Attaching new patch with minor changes and adding testcase.

        Show
        Ravi Gummadi added a comment - Attaching new patch with minor changes and adding testcase.
        Hide
        Ravi Gummadi added a comment -

        Attaching new patch incorporating minor comments from Amar.

        Show
        Ravi Gummadi added a comment - Attaching new patch incorporating minor comments from Amar.
        Hide
        Ravi Gummadi added a comment -

        Unit tests passed on my local machine.

        ant test-patch gave:

        [exec] +1 overall.
        [exec]
        [exec] +1 @author. The patch does not contain any @author tags.
        [exec]
        [exec] +1 tests included. The patch appears to include 3 new or modified tests.
        [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] +1 system tests framework. The patch passed system tests framework compile.

        Show
        Ravi Gummadi added a comment - Unit tests passed on my local machine. ant test-patch gave: [exec] +1 overall. [exec] [exec] +1 @author. The patch does not contain any @author tags. [exec] [exec] +1 tests included. The patch appears to include 3 new or modified tests. [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] +1 system tests framework. The patch passed system tests framework compile.
        Hide
        Amar Kamat added a comment -

        Few minor comments:

        1. In GridMix.java, userResolver.getClass().getSimpleName() should be userResolver.getClass().toString(). The reason being, there can be a user defined class RoundRobinResolver but in a different package.
        2. In the testcase, there is an assumption that writeFile() will create the test root directories and also create the file. One good way of writing the testcase would be
          public void testRoundRobinResolver() throws Exception {
          	 String root = initializeTempFile();
                   Path userListFile = new Path(root, "users.txt")
                   
                   // test with missing users file
                   .....
          
                   // test with empty users file
                   writeUserList(userListFile, "");
                   ...
          
                  // test with valid contents
                  writeUserList(userListFile, "user,group\n");
          
        3. testSubmitterResolver() and other resolvers should ideally test the return value of needsTargetUsersList() and bail out. If needsTargetUsersList() returns false then there is no point in testing setTargetUsers() as their behavior is undefined.

        Rest of the patch seems good.

        Show
        Amar Kamat added a comment - Few minor comments: In GridMix.java , userResolver.getClass().getSimpleName() should be userResolver.getClass().toString() . The reason being, there can be a user defined class RoundRobinResolver but in a different package. In the testcase, there is an assumption that writeFile() will create the test root directories and also create the file. One good way of writing the testcase would be public void testRoundRobinResolver() throws Exception { String root = initializeTempFile(); Path userListFile = new Path(root, "users.txt" ) // test with missing users file ..... // test with empty users file writeUserList(userListFile, ""); ... // test with valid contents writeUserList(userListFile, "user,group\n" ); testSubmitterResolver() and other resolvers should ideally test the return value of needsTargetUsersList() and bail out. If needsTargetUsersList() returns false then there is no point in testing setTargetUsers() as their behavior is undefined. Rest of the patch seems good.
        Hide
        Ravi Gummadi added a comment -

        Attaching patch incorporating review comments and fixing SubmitterUserResolver to set ugi in the constructor itself as setTargetUsers() would not be called because needsTargetUsersList() returns false.

        Show
        Ravi Gummadi added a comment - Attaching patch incorporating review comments and fixing SubmitterUserResolver to set ugi in the constructor itself as setTargetUsers() would not be called because needsTargetUsersList() returns false.
        Hide
        Ravi Gummadi added a comment -

        Previous patch has some build.xml changes, which are not related to this issue. Updated patch is here...

        Show
        Ravi Gummadi added a comment - Previous patch has some build.xml changes, which are not related to this issue. Updated patch is here...
        Hide
        Amar Kamat added a comment -

        Looks good. +1.

        Show
        Amar Kamat added a comment - Looks good. +1.
        Hide
        Ravi Gummadi added a comment -

        Unit tests passed on my local machine.

        ant test-patch gave:

        [exec] +1 overall.
        [exec]
        [exec] +1 @author. The patch does not contain any @author tags.
        [exec]
        [exec] +1 tests included. The patch appears to include 3 new or modified tests.
        [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] +1 system tests framework. The patch passed system tests framework compile.

        Show
        Ravi Gummadi added a comment - Unit tests passed on my local machine. ant test-patch gave: [exec] +1 overall. [exec] [exec] +1 @author. The patch does not contain any @author tags. [exec] [exec] +1 tests included. The patch appears to include 3 new or modified tests. [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] +1 system tests framework. The patch passed system tests framework compile.
        Hide
        Amareshwari Sriramadasu added a comment -

        I just committed this. Thanks Ravi !

        Show
        Amareshwari Sriramadasu added a comment - I just committed this. Thanks Ravi !
        Hide
        Hudson added a comment -

        Integrated in Hadoop-Mapreduce-trunk-Commit #523 (See https://hudson.apache.org/hudson/job/Hadoop-Mapreduce-trunk-Commit/523/)

        Show
        Hudson added a comment - Integrated in Hadoop-Mapreduce-trunk-Commit #523 (See https://hudson.apache.org/hudson/job/Hadoop-Mapreduce-trunk-Commit/523/ )

          People

          • Assignee:
            Ravi Gummadi
            Reporter:
            Ravi Gummadi
          • Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development