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

CompletedJobStatusStore initialization should fail if {mapred.job.tracker.persist.jobstatus.dir} is unwritable

    Details

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

      Description

      If

      {mapred.job.tracker.persist.jobstatus.dir} points to an unwritable location or mkdir of {mapred.job.tracker.persist.jobstatus.dir}

      fails, then CompletedJobStatusStore silently ignores the failure and disables CompletedJobStatusStore. Ideally the JobTracker should bail out early indicating a misconfiguration.

      1. mapred-1778.20S.patch
        4 kB
        Krishna Ramachandran
      2. mapred-1778.20S-1.patch
        4 kB
        Krishna Ramachandran
      3. mapred-1778.patch
        6 kB
        Krishna Ramachandran
      4. mapred-1778-1.patch
        6 kB
        Krishna Ramachandran
      5. mapred-1778-2.patch
        6 kB
        Krishna Ramachandran
      6. mapred-1778-3.patch
        6 kB
        Krishna Ramachandran
      7. mapred-1778-4.patch
        4 kB
        Krishna Ramachandran

        Activity

        Amar Kamat created issue -
        Amar Kamat made changes -
        Field Original Value New Value
        Assignee Amar Kamat [ amar_kamat ]
        Hide
        Krishna Ramachandran added a comment -

        Potential fix for trunk

        Show
        Krishna Ramachandran added a comment - Potential fix for trunk
        Krishna Ramachandran made changes -
        Attachment mapred-1778.patch [ 12445802 ]
        Hide
        Krishna Ramachandran added a comment -

        Suggested fix + test case

        Show
        Krishna Ramachandran added a comment - Suggested fix + test case
        Krishna Ramachandran made changes -
        Status Open [ 1 ] Patch Available [ 10002 ]
        Hide
        Hadoop QA added a comment -

        -1 overall. Here are the results of testing the latest attachment
        http://issues.apache.org/jira/secure/attachment/12445802/mapred-1778.patch
        against trunk revision 949090.

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

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

        -1 patch. The patch command could not apply the patch.

        Console output: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h4.grid.sp2.yahoo.net/209/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/12445802/mapred-1778.patch against trunk revision 949090. +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 2 new or modified tests. -1 patch. The patch command could not apply the patch. Console output: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h4.grid.sp2.yahoo.net/209/console This message is automatically generated.
        Krishna Ramachandran made changes -
        Status Patch Available [ 10002 ] Open [ 1 ]
        Krishna Ramachandran made changes -
        Attachment mapred-1778-1.patch [ 12445821 ]
        Krishna Ramachandran made changes -
        Status Open [ 1 ] Patch Available [ 10002 ]
        Hide
        Hadoop QA added a comment -

        -1 overall. Here are the results of testing the latest attachment
        http://issues.apache.org/jira/secure/attachment/12445821/mapred-1778-1.patch
        against trunk revision 949090.

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

        +1 tests included. The patch appears to include 2 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 failed core unit tests.

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

        Test results: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h4.grid.sp2.yahoo.net/210/testReport/
        Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h4.grid.sp2.yahoo.net/210/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
        Checkstyle results: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h4.grid.sp2.yahoo.net/210/artifact/trunk/build/test/checkstyle-errors.html
        Console output: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h4.grid.sp2.yahoo.net/210/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/12445821/mapred-1778-1.patch against trunk revision 949090. +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 2 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 failed core unit tests. +1 contrib tests. The patch passed contrib unit tests. Test results: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h4.grid.sp2.yahoo.net/210/testReport/ Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h4.grid.sp2.yahoo.net/210/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Checkstyle results: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h4.grid.sp2.yahoo.net/210/artifact/trunk/build/test/checkstyle-errors.html Console output: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h4.grid.sp2.yahoo.net/210/console This message is automatically generated.
        Hide
        Amar Kamat added a comment -

        Krishna,
        There are 2 cases here
        1) If mkdir() fails
        2) If mapred.job.tracker.persist.jobstatus.dir exists but is not writable

        We need to fix both these cases. From the latest patch, I see that you have fixed #1. For #2, I think maybe we can try and create a dummy temp file. Is there a better way than this? Did not find any helper api in FileSystem.

        Comments on the testcases :
        1) Can you kindly remove commented code and add javadoc comments for the testcases.
        2) Instead of adding a new testcase, its better that we modify the existing testcase i.e TestJobStatusPersistency. Here you can change testJobStoreDisablingWithInvalidPath() (rename it to testJobStoreWithInvalidPath) to fail when an invalid path is provided instead of disabling. You can add a new test (say testJobStoreWithUnwritablePath()) for testing #2.

        Show
        Amar Kamat added a comment - Krishna, There are 2 cases here 1) If mkdir() fails 2) If mapred.job.tracker.persist.jobstatus.dir exists but is not writable We need to fix both these cases. From the latest patch , I see that you have fixed #1. For #2, I think maybe we can try and create a dummy temp file. Is there a better way than this? Did not find any helper api in FileSystem. Comments on the testcases : 1) Can you kindly remove commented code and add javadoc comments for the testcases. 2) Instead of adding a new testcase, its better that we modify the existing testcase i.e TestJobStatusPersistency. Here you can change testJobStoreDisablingWithInvalidPath() (rename it to testJobStoreWithInvalidPath) to fail when an invalid path is provided instead of disabling. You can add a new test (say testJobStoreWithUnwritablePath()) for testing #2.
        Hide
        Hemanth Yamijala added a comment -

        For #2, I think maybe we can try and create a dummy temp file. Is there a better way than this? Did not find any helper api in FileSystem.

        If you had the FileStatus for the directory, you could use FsPermission to check this, no ? But if you don't have it, you'd need an additional RPC for the same.

        Show
        Hemanth Yamijala added a comment - For #2, I think maybe we can try and create a dummy temp file. Is there a better way than this? Did not find any helper api in FileSystem. If you had the FileStatus for the directory, you could use FsPermission to check this, no ? But if you don't have it, you'd need an additional RPC for the same.
        Hide
        Krishna Ramachandran added a comment -

        Amar
        I agree there are 2 possibilities - have not checked as to how to address item 2 (if directory though exists can not be written to)
        will update

        Show
        Krishna Ramachandran added a comment - Amar I agree there are 2 possibilities - have not checked as to how to address item 2 (if directory though exists can not be written to) will update
        Hide
        Chris Douglas added a comment -

        Canceling patch while Amar and Hemanth's comments are addressed.

        The unit test contains a lot of commented-out code that should be removed. It also starts Mini*Clusters instead of testing the CompletedJobStatusStore cstr directly, which is sufficient, isn't it?

        Show
        Chris Douglas added a comment - Canceling patch while Amar and Hemanth's comments are addressed. The unit test contains a lot of commented-out code that should be removed. It also starts Mini*Clusters instead of testing the CompletedJobStatusStore cstr directly, which is sufficient, isn't it?
        Chris Douglas made changes -
        Status Patch Available [ 10002 ] Open [ 1 ]
        Hide
        Krishna Ramachandran added a comment -

        I am testing a fix for item 2 using FsPermission - will resubmit tomorrow

        Show
        Krishna Ramachandran added a comment - I am testing a fix for item 2 using FsPermission - will resubmit tomorrow
        Hide
        Krishna Ramachandran added a comment -

        Revised patch based on input from Amar, Hemanth and Chris

        1) Check for valid file permissions for StatusStore
        2) Throw error if directory exists and permissions are incorrect
        3) updated test case

        javadoc
        clean up commented code
        negative and positive tests

        Setting incorrect permissions will cause JobTracker to fail as captured by test case

        2010-06-07 15:50:29,798 WARN mapred.JobTracker (JobTracker.java:startTracker(257)) - Error starting tracker: java.io.IOException: JobStatusStore permissions are incorrect

        Show
        Krishna Ramachandran added a comment - Revised patch based on input from Amar, Hemanth and Chris 1) Check for valid file permissions for StatusStore 2) Throw error if directory exists and permissions are incorrect 3) updated test case javadoc clean up commented code negative and positive tests Setting incorrect permissions will cause JobTracker to fail as captured by test case 2010-06-07 15:50:29,798 WARN mapred.JobTracker (JobTracker.java:startTracker(257)) - Error starting tracker: java.io.IOException: JobStatusStore permissions are incorrect
        Krishna Ramachandran made changes -
        Attachment mapred-1778-2.patch [ 12446543 ]
        Hide
        Krishna Ramachandran added a comment -

        Attached revised patch

        Show
        Krishna Ramachandran added a comment - Attached revised patch
        Krishna Ramachandran made changes -
        Status Open [ 1 ] Patch Available [ 10002 ]
        Hide
        Hadoop QA added a comment -

        -1 overall. Here are the results of testing the latest attachment
        http://issues.apache.org/jira/secure/attachment/12446543/mapred-1778-2.patch
        against trunk revision 952460.

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

        +1 tests included. The patch appears to include 2 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 failed core unit tests.

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

        Test results: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h4.grid.sp2.yahoo.net/227/testReport/
        Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h4.grid.sp2.yahoo.net/227/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
        Checkstyle results: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h4.grid.sp2.yahoo.net/227/artifact/trunk/build/test/checkstyle-errors.html
        Console output: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h4.grid.sp2.yahoo.net/227/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/12446543/mapred-1778-2.patch against trunk revision 952460. +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 2 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 failed core unit tests. -1 contrib tests. The patch failed contrib unit tests. Test results: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h4.grid.sp2.yahoo.net/227/testReport/ Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h4.grid.sp2.yahoo.net/227/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Checkstyle results: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h4.grid.sp2.yahoo.net/227/artifact/trunk/build/test/checkstyle-errors.html Console output: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h4.grid.sp2.yahoo.net/227/console This message is automatically generated.
        Hide
        Amar Kamat added a comment -

        Some comments
        1) Bug related:
        1.1) I think the newly created completed-jobstatus-store directory should assume permissions from the parent dir instead of hardcoding it to 750.
        1.2) If completed-jobstatus-store already exists then the only thing that should be tested/verified is whether its writable+readable by jobtracker or not. It doesnt really matter what the parent/group/other's permissions really are as long as the JobTracker can get its work (read+write) done.

        2) Coding related
        2.1) Make sure that the code is aligned within 80 characters limit (i.e column width)

        3) Testcase related
        3.1) Use log4j instead of mortbay logger
        3.2) Add descriptive javadoc for the testcase. Mention that the scope of the test is only limited to JobTracker's usage of CompletedJobStatusStore.
        3.3) I still see some commented out code in the testcase. Plz remove it.
        3.4) I would prefer using the existing testcase (i.e TestJobStatusPersistency.java) instead of introducing a new testcase (i.e TestJobStatusStoreConfig.java). See http://tinyurl.com/24kzetd.
        3.5) [Minor] Indentation related : See line 166,167 of the patch

        Show
        Amar Kamat added a comment - Some comments 1) Bug related: 1.1) I think the newly created completed-jobstatus-store directory should assume permissions from the parent dir instead of hardcoding it to 750. 1.2) If completed-jobstatus-store already exists then the only thing that should be tested/verified is whether its writable+readable by jobtracker or not. It doesnt really matter what the parent/group/other's permissions really are as long as the JobTracker can get its work (read+write) done. 2) Coding related 2.1) Make sure that the code is aligned within 80 characters limit (i.e column width) 3) Testcase related 3.1) Use log4j instead of mortbay logger 3.2) Add descriptive javadoc for the testcase. Mention that the scope of the test is only limited to JobTracker's usage of CompletedJobStatusStore. 3.3) I still see some commented out code in the testcase. Plz remove it. 3.4) I would prefer using the existing testcase (i.e TestJobStatusPersistency.java) instead of introducing a new testcase (i.e TestJobStatusStoreConfig.java). See http://tinyurl.com/24kzetd . 3.5) [Minor] Indentation related : See line 166,167 of the patch
        Hide
        Krishna Ramachandran added a comment -

        If the directory exists and permissions are correct jt can read from or write to - is this not correct?

        Permissions are similar to what is done in JobHistory data

        The only other way at least i know is to create a temporary file - something like

        if (!exists) create;

        else

        { Path testpath = new Path(path, "testfile"); DataOutputStream out = fs.create(testpath); out.writeBytes("testdata"); out.close(); }

        Chris is not in favor. If there is a better way let me know. Though this is a one time thing (when jt starts)

        I used hadoop code formatter - will make sure alignments are correct

        As for test case, scope of JobStatusPersistency is different would rather have a separate one

        Show
        Krishna Ramachandran added a comment - If the directory exists and permissions are correct jt can read from or write to - is this not correct? Permissions are similar to what is done in JobHistory data The only other way at least i know is to create a temporary file - something like if (!exists) create; else { Path testpath = new Path(path, "testfile"); DataOutputStream out = fs.create(testpath); out.writeBytes("testdata"); out.close(); } Chris is not in favor. If there is a better way let me know. Though this is a one time thing (when jt starts) I used hadoop code formatter - will make sure alignments are correct As for test case, scope of JobStatusPersistency is different would rather have a separate one
        Hide
        Amar Kamat added a comment -

        If the directory exists and permissions are correct jt can read from or write to - is this not correct?

        How do you define the phrase "permissions are correct"?

        • If the directory matches predefined and expected permissions
        • if its writable and readable

        The good part about #2 is that no matter what the directory permissions, the verification code is simply concerned with the job at hand i.e to test if the dir is r+w. #1 wont work is completedjob-status-store dir is pre created with different permissions.

        As for test case, scope of JobStatusPersistency is different would rather have a separate one

        Krishna, there is a test scenario in TestJobStatusPersistency.java i.e testJobStoreDisablingWithInvalidPath, which checks exactly the same thing as we intend to check here, except for the fact that instead of silently disabling CompletedJobStatusStore, we should check if the jobtracker bails out. TestJobStatusPersistency might sound as a misnomer but its exactly the feature provided by the CompletedJobStatusStore.java.

        Show
        Amar Kamat added a comment - If the directory exists and permissions are correct jt can read from or write to - is this not correct? How do you define the phrase "permissions are correct"? If the directory matches predefined and expected permissions if its writable and readable The good part about #2 is that no matter what the directory permissions, the verification code is simply concerned with the job at hand i.e to test if the dir is r+w. #1 wont work is completedjob-status-store dir is pre created with different permissions. As for test case, scope of JobStatusPersistency is different would rather have a separate one Krishna, there is a test scenario in TestJobStatusPersistency.java i.e testJobStoreDisablingWithInvalidPath, which checks exactly the same thing as we intend to check here, except for the fact that instead of silently disabling CompletedJobStatusStore, we should check if the jobtracker bails out. TestJobStatusPersistency might sound as a misnomer but its exactly the feature provided by the CompletedJobStatusStore.java.
        Hide
        Krishna Ramachandran added a comment -

        "if its writable and readable"

        I have NO knowledge of any API that will check for this (other than creating a dummy file)
        Look at JobHistory - what i have done is similar.

        Chris,
        Can you comment?

        I fixed the line alignment per hadoop formatter

        As for test case, I let me check and if relevant use the existing test

        Show
        Krishna Ramachandran added a comment - "if its writable and readable" I have NO knowledge of any API that will check for this (other than creating a dummy file) Look at JobHistory - what i have done is similar. Chris, Can you comment? I fixed the line alignment per hadoop formatter As for test case, I let me check and if relevant use the existing test
        Hide
        Krishna Ramachandran added a comment -

        Fix alignment

        take out references to mortbay logger

        Neither the last rev nor the current one has any commented code to remove
        If I missed something let me know

        Leaving this new test for now and will update if existing one can be leveraged

        Show
        Krishna Ramachandran added a comment - Fix alignment take out references to mortbay logger Neither the last rev nor the current one has any commented code to remove If I missed something let me know Leaving this new test for now and will update if existing one can be leveraged
        Krishna Ramachandran made changes -
        Attachment mapred-1778-3.patch [ 12446618 ]
        Hide
        Amar Kamat added a comment -

        Neither the last rev nor the current one has any commented code to remove

        public class TestJobStatusStoreConfig extends TestCase {
        +  // private MiniMRCluster mr = null;
        

        and

        conf.set("mapred.job.tracker.persist.jobstatus.dir", "/jobsInfo/stat");
        +      /*
        +       */
        +      Path logDir = new Path("/jobsInfo/stat");
        
        Show
        Amar Kamat added a comment - Neither the last rev nor the current one has any commented code to remove public class TestJobStatusStoreConfig extends TestCase { + // private MiniMRCluster mr = null ; and conf.set( "mapred.job.tracker.persist.jobstatus.dir" , "/jobsInfo/stat" ); + /* + */ + Path logDir = new Path( "/jobsInfo/stat" );
        Hide
        Amar Kamat added a comment -

        Krishna,
        What would happen if the completed-job-status-store is pre-created as a file instead of a directory? This should be considered as a mis-configuration and the JobTracker should bail out. WIth the latest patch, the JobTracker would come up fine. Looks like a bug in the trunk too. Also kindly add this as another test-scenario.

        Show
        Amar Kamat added a comment - Krishna, What would happen if the completed-job-status-store is pre-created as a file instead of a directory? This should be considered as a mis-configuration and the JobTracker should bail out. WIth the latest patch, the JobTracker would come up fine. Looks like a bug in the trunk too. Also kindly add this as another test-scenario.
        Hide
        Arun C Murthy added a comment -

        Krishna, here is sample code to check for read/write perms:

            FileStatus stat = fs.getFileStatus(dir);
            FsPermission actual = stat.getPermission();
            
            if (!stat.isDir())
              throw new DiskErrorException("not a directory: " 
                                           + dir.toString());
                    
            FsAction user = actual.getUserAction();
            if (!user.implies(FsAction.READ))
              throw new DiskErrorException("directory is not readable: " 
                                           + dir.toString());
                    
            if (!user.implies(FsAction.WRITE))
              throw new DiskErrorException("directory is not writable: " 
                                           + dir.toString());
        

        This idiom is better than relying a fixed permission.

        Show
        Arun C Murthy added a comment - Krishna, here is sample code to check for read/write perms: FileStatus stat = fs.getFileStatus(dir); FsPermission actual = stat.getPermission(); if (!stat.isDir()) throw new DiskErrorException( "not a directory: " + dir.toString()); FsAction user = actual.getUserAction(); if (!user.implies(FsAction.READ)) throw new DiskErrorException( "directory is not readable: " + dir.toString()); if (!user.implies(FsAction.WRITE)) throw new DiskErrorException( "directory is not writable: " + dir.toString()); This idiom is better than relying a fixed permission.
        Arun C Murthy made changes -
        Status Patch Available [ 10002 ] Open [ 1 ]
        Assignee Amar Kamat [ amar_kamat ] Krishna Ramachandran [ ramach ]
        Hide
        Amar Kamat added a comment -

        Krishna, here is sample code to check for read/write perms: ...

        +1. Use stat.isDirectory() as stat.isDir() is deprecated.

        Show
        Amar Kamat added a comment - Krishna, here is sample code to check for read/write perms: ... +1. Use stat.isDirectory() as stat.isDir() is deprecated.
        Hide
        Krishna Ramachandran added a comment -

        Amar
        I only see FileStatus.isDir()

        in trunk (org.apache.hadoop.fs)

        /**

        • Is this a directory?
        • @return true if this is a directory
          */
          public boolean isDir() { return isdir; }
        Show
        Krishna Ramachandran added a comment - Amar I only see FileStatus.isDir() in trunk (org.apache.hadoop.fs) /** Is this a directory? @return true if this is a directory */ public boolean isDir() { return isdir; }
        Hide
        Krishna Ramachandran added a comment -

        Update

        Include Arun's changes

        Removed the new test and modified existing (from Amar) TestJobStatusPersistency.java

        Show
        Krishna Ramachandran added a comment - Update Include Arun's changes Removed the new test and modified existing (from Amar) TestJobStatusPersistency.java
        Krishna Ramachandran made changes -
        Attachment mapred-1778-4.patch [ 12446825 ]
        Hide
        Krishna Ramachandran added a comment -

        From Arun's review (since last patch)

        Check read write permissions using FsAction.getuserAction();

        Amar's reco

        Modified TestJobStatusPersistency.java

        Show
        Krishna Ramachandran added a comment - From Arun's review (since last patch) Check read write permissions using FsAction.getuserAction(); Amar's reco Modified TestJobStatusPersistency.java
        Krishna Ramachandran made changes -
        Status Open [ 1 ] Patch Available [ 10002 ]
        Hide
        Amar Kamat added a comment -

        I only see FileStatus.isDir()

        Look at FileStatus.java from hadoop-common.

        Comments:

        1. I still see few lines of code crossing the 80 column margin.
        2. Plz add comments. For example
          +      } else {
          +        FileStatus stat = fs.getFileStatus(path);
          +        FsPermission actual = stat.getPermission();
          +        if (!stat.isDir())
          +          throw new DiskErrorException("not a directory: " 
          +                                   + path.toString());
          +        FsAction user = actual.getUserAction();
          +        if (!user.implies(FsAction.READ))
          +          throw new DiskErrorException("directory is not readable: "
          +                                   + path.toString());
          +        if (!user.implies(FsAction.WRITE))
          +          throw new DiskErrorException("directory is not writable: "
          +                                   + path.toString());
          

          It would be nice to explain why its done this way.

        Testcase:

        1. It would be better if we test the following scenarios
          1. directory doesnt exist but mkdir fails() [example: parent folder doesnt have write permissions?]
          2. directory exists but no read/write permissions
          3. completedjob status store is a file and not a directory
            I think the current changes in the patch tests sceanrio#2. Can you please add tests for testing scenario#1 and scenario#3?
        2. If the JobTracker gets started, it should be shut down.
        3. For a newly created job-status-store dir, it would be nice to validate its permissions (i.e JOB_STATUS_STORE_DIR_PERMISSION).
        Show
        Amar Kamat added a comment - I only see FileStatus.isDir() Look at FileStatus.java from hadoop-common. Comments: I still see few lines of code crossing the 80 column margin. Plz add comments. For example + } else { + FileStatus stat = fs.getFileStatus(path); + FsPermission actual = stat.getPermission(); + if (!stat.isDir()) + throw new DiskErrorException( "not a directory: " + + path.toString()); + FsAction user = actual.getUserAction(); + if (!user.implies(FsAction.READ)) + throw new DiskErrorException( "directory is not readable: " + + path.toString()); + if (!user.implies(FsAction.WRITE)) + throw new DiskErrorException( "directory is not writable: " + + path.toString()); It would be nice to explain why its done this way. Testcase: It would be better if we test the following scenarios directory doesnt exist but mkdir fails() [example: parent folder doesnt have write permissions?] directory exists but no read/write permissions completedjob status store is a file and not a directory I think the current changes in the patch tests sceanrio#2. Can you please add tests for testing scenario#1 and scenario#3? If the JobTracker gets started, it should be shut down. For a newly created job-status-store dir, it would be nice to validate its permissions (i.e JOB_STATUS_STORE_DIR_PERMISSION).
        Hide
        Hadoop QA added a comment -

        -1 overall. Here are the results of testing the latest attachment
        http://issues.apache.org/jira/secure/attachment/12446825/mapred-1778-4.patch
        against trunk revision 953490.

        +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 failed core unit tests.

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

        Test results: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h4.grid.sp2.yahoo.net/233/testReport/
        Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h4.grid.sp2.yahoo.net/233/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
        Checkstyle results: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h4.grid.sp2.yahoo.net/233/artifact/trunk/build/test/checkstyle-errors.html
        Console output: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h4.grid.sp2.yahoo.net/233/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/12446825/mapred-1778-4.patch against trunk revision 953490. +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 failed core unit tests. -1 contrib tests. The patch failed contrib unit tests. Test results: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h4.grid.sp2.yahoo.net/233/testReport/ Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h4.grid.sp2.yahoo.net/233/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Checkstyle results: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h4.grid.sp2.yahoo.net/233/artifact/trunk/build/test/checkstyle-errors.html Console output: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h4.grid.sp2.yahoo.net/233/console This message is automatically generated.
        Hide
        Krishna Ramachandran added a comment -

        Fix for an earlier release
        not for commit here

        Show
        Krishna Ramachandran added a comment - Fix for an earlier release not for commit here
        Krishna Ramachandran made changes -
        Attachment mapred-1778.20S.patch [ 12446889 ]
        Hide
        Krishna Ramachandran added a comment -

        revised 20S patch (git pull) after repo sync

        Show
        Krishna Ramachandran added a comment - revised 20S patch (git pull) after repo sync
        Krishna Ramachandran made changes -
        Attachment mapred-1778.20S-1.patch [ 12447100 ]
        Hide
        Arun C Murthy added a comment -

        I just committed this. Thanks Krishna!

        Show
        Arun C Murthy added a comment - I just committed this. Thanks Krishna!
        Arun C Murthy made changes -
        Status Patch Available [ 10002 ] Resolved [ 5 ]
        Hadoop Flags [Reviewed]
        Resolution Fixed [ 1 ]
        Hide
        Nigel Daley added a comment -

        The commit log/comment for this Jira is on MAPREDUCE-1788 due to a mistake in the commit comment.

        Show
        Nigel Daley added a comment - The commit log/comment for this Jira is on MAPREDUCE-1788 due to a mistake in the commit comment.
        Hide
        Eli Collins added a comment -

        I fixed CHANGES.txt on trunk and 22.

        Show
        Eli Collins added a comment - I fixed CHANGES.txt on trunk and 22.
        Hide
        Hudson added a comment -

        Integrated in Hadoop-Mapreduce-22-branch #50 (See https://builds.apache.org/hudson/job/Hadoop-Mapreduce-22-branch/50/)
        MAPREDUCE-1778. svn merge -c 1102898 from trunk

        Show
        Hudson added a comment - Integrated in Hadoop-Mapreduce-22-branch #50 (See https://builds.apache.org/hudson/job/Hadoop-Mapreduce-22-branch/50/ ) MAPREDUCE-1778 . svn merge -c 1102898 from trunk
        Hide
        Hudson added a comment -

        Integrated in Hadoop-Mapreduce-trunk #679 (See https://builds.apache.org/hudson/job/Hadoop-Mapreduce-trunk/679/)
        Update CHANGES.txt to reflect MAPREDUCE-1778 was committed.

        Show
        Hudson added a comment - Integrated in Hadoop-Mapreduce-trunk #679 (See https://builds.apache.org/hudson/job/Hadoop-Mapreduce-trunk/679/ ) Update CHANGES.txt to reflect MAPREDUCE-1778 was committed.
        Hide
        Hudson added a comment -

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

        Show
        Hudson added a comment - Integrated in Hadoop-Mapreduce-trunk-Commit #675 (See https://builds.apache.org/hudson/job/Hadoop-Mapreduce-trunk-Commit/675/ )
        Konstantin Shvachko made changes -
        Fix Version/s 0.22.0 [ 12314184 ]
        Konstantin Shvachko made changes -
        Status Resolved [ 5 ] Closed [ 6 ]

          People

          • Assignee:
            Krishna Ramachandran
            Reporter:
            Amar Kamat
          • Votes:
            0 Vote for this issue
            Watchers:
            2 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development