MAPREDUCE-7375 JobSubmissionFiles don't set right permission after mkdirs #4237
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
skysiders
changed the title
skysiders
changed the title
|
🎊 +1 overall
This message was automatically generated. |
|
In this PR , I set permission for directory with special permission after mkdir, so that the directory can has right permission |
|
Hi @iwasakims ,could you please take a look? Thanks |
|
Hi @cnauroth .Could you please take a look? Thanks |
cnauroth
requested changes
Jul 28, 2022
| fs.setPermission(stagingArea, JOB_DIR_PERMISSION); | ||
| } | ||
| } catch (FileNotFoundException e) { | ||
| fs.mkdirs(stagingArea, new FsPermission(JOB_DIR_PERMISSION)); |
There was a problem hiding this comment.
I suggest using static method FileSystem.mkdirs(fs, stagingArea, new FsPermission(JOB_DIR_PERMISSION)). It's one less line of code and potentially one less remote call for certain FileSystem implementations.
cnauroth
requested changes
Jul 28, 2022
| @Test | ||
| public void testDirPermission() throws Exception { | ||
| Cluster cluster = mock(Cluster.class); | ||
| HdfsConfiguration CONF = new HdfsConfiguration(); |
There was a problem hiding this comment.
Nitpick: lower-case conf for local variables.
| public void testDirPermission() throws Exception { | ||
| Cluster cluster = mock(Cluster.class); | ||
| HdfsConfiguration CONF = new HdfsConfiguration(); | ||
| MiniDFSCluster cluster1 = new MiniDFSCluster.Builder(CONF).numDataNodes(2).build(); |
There was a problem hiding this comment.
Suggestion: different variable name for greater clarity, e.g. dfsCluster.
Also, please use a finally block to guarantee calling MiniDFSCluster#shutdown() at the end of the test.
|
|
||
| when(cluster.getStagingAreaDir()).thenReturn(stagingPath); | ||
| Path res = JobSubmissionFiles.getStagingDir(cluster, CONF, user); | ||
| assertEquals(new FsPermission(0700),fs.getFileStatus(res).getPermission()); |
There was a problem hiding this comment.
Nitpick: please add a single space after the comma.
...-mapreduce-client-core/src/test/java/org/apache/hadoop/mapreduce/TestJobSubmissionFiles.java
Show resolved
Hide resolved
|
🎊 +1 overall
This message was automatically generated. |
|
Hi @cnauroth , thanks for your review. I'm sorry for the problems here, I've fixed them. |
|
🎊 +1 overall
This message was automatically generated. |
cnauroth
requested changes
Jul 29, 2022
...-mapreduce-client-core/src/test/java/org/apache/hadoop/mapreduce/TestJobSubmissionFiles.java
Outdated
Show resolved
Hide resolved
|
💔 -1 overall
This message was automatically generated. |
cnauroth
approved these changes
Jan 7, 2023
There was a problem hiding this comment.
+1
I'll plan on committing this to trunk, branch-3.3 and branch-3.2 by next week. @skysiders , sorry for my delayed response. I was losing my GitHub notifications to the spam folder for a while.
|
I have committed this to trunk, branch-3.3 and branch-3.2. @skysiders , thank you for the contribution. |
|
how important is this for the next 3.3.5 RC |
|
@steveloughran , I don't think this one is a strong candidate for the next 3.3.5 RC. |
|
okay, let's cherrypick. |
|
sorry, @cnauroth , misread your comment and put it in -i don't think it is worth reverting now |
|
@steveloughran , no problem, thanks! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
MAPREDUCE-7375 JobSubmissionFiles don't set right permission after mkdirs
Description of PR
JobSubmissionFiles provide getStagingDir to get Staging Directory.If stagingArea missing, method will create new directory with this.
fs.mkdirs(stagingArea, new FsPermission(JOB_DIR_PERMISSION));
It seems create new directory with JOB_DIR_PERMISSION,but this permission will be apply by umask.If umask too strict , this permission may be 000(if umask is 700).So we should change permission after create.
How was this patch tested?
For code changes:
LICENSE,LICENSE-binary,NOTICE-binaryfiles?