TEZ-4412 ensure mkDirForAM create directory with special permissions by skysiders · Pull Request #209 · apache/tez

skysiders · 2022-05-03T11:57:46Z

TEZ-4412 ensure mkDirForAM create directory with special permissions

I found TezClientUtils has method ensureStagingDirExists.It will check whether the path "stagingArea" exists, if it exists, check the permission, if not, call the function "TezCommonUtils.mkDirForAM" to create.But in method mkDirForAM,it use mkdir(path, permission) to create, if umask too strict such as 777,this directory will set with 000 permission.So we need to ensure the directory has right permission.

In this patch, I compared the permissions of the files with the expected permissions. If they are inconsistent, then log this and use setPermission to grant directory permissions.

skysiders · 2022-05-05T07:59:21Z

Hi @abstractdog ,I found some permission issue here, can you please take a look?

abstractdog · 2022-05-07T08:00:19Z

thanks for the patch @skysiders!
looks good to me, left a minor comment, please take care of it as well as checkstyle warnings

abstractdog · 2022-05-07T07:59:13Z

- fs.mkdirs(dir, new FsPermission(TEZ_AM_DIR_PERMISSION));
+ FsPermission perm = new FsPermission(TEZ_AM_DIR_PERMISSION);
+ fs.mkdirs(dir, perm);
+ if(!fs.getFileStatus(dir).getPermission().equals(perm)){


this method is public static, looks easily testable, could you please introduce a unit test in TestTezCommonUtils?

Thanks for your review. I will check it.

Hi @abstractdog ,I add UT and fix check style. Couldyou please take a look?

skysiders · 2022-05-07T09:15:24Z

this test is properly working for the original scenario, can you include another one that tests the scenario you mentioned (umask)?

Hi @abstractdog , I'm sorry to make mistake in UT, and I try to fix it in this patch.

tez-yetus · 2022-05-07T09:45:19Z

🎊 +1 overall

Vote	Subsystem	Runtime	Comment
+0 🆗	reexec	1m 2s	Docker mode activated.
		_ Prechecks _
+1 💚	dupname	0m 0s	No case conflicting files found.
+1 💚	@author	0m 0s	The patch does not contain any @author tags.
+1 💚	test4tests	0m 0s	The patch appears to include 1 new or modified test files.
		_ master Compile Tests _
+1 💚	mvninstall	17m 5s	master passed
+1 💚	compile	0m 44s	master passed with JDK Private Build-11.0.15+10-Ubuntu-0ubuntu0.20.04.1
+1 💚	compile	0m 41s	master passed with JDK Private Build-1.8.0_312-8u312-b07-0ubuntu1~20.04-b07
+1 💚	checkstyle	1m 10s	master passed
+1 💚	javadoc	0m 56s	master passed with JDK Private Build-11.0.15+10-Ubuntu-0ubuntu0.20.04.1
+1 💚	javadoc	0m 44s	master passed with JDK Private Build-1.8.0_312-8u312-b07-0ubuntu1~20.04-b07
+0 🆗	spotbugs	1m 39s	Used deprecated FindBugs config; considering switching to SpotBugs.
+1 💚	findbugs	1m 37s	master passed
		_ Patch Compile Tests _
+1 💚	mvninstall	0m 23s	the patch passed
+1 💚	compile	0m 26s	the patch passed with JDK Private Build-11.0.15+10-Ubuntu-0ubuntu0.20.04.1
+1 💚	javac	0m 26s	the patch passed
+1 💚	compile	0m 21s	the patch passed with JDK Private Build-1.8.0_312-8u312-b07-0ubuntu1~20.04-b07
+1 💚	javac	0m 21s	the patch passed
-0 ⚠️	checkstyle	0m 13s	tez-api: The patch generated 1 new + 23 unchanged - 0 fixed = 24 total (was 23)
+1 💚	whitespace	0m 0s	The patch has no whitespace issues.
+1 💚	javadoc	0m 25s	the patch passed with JDK Private Build-11.0.15+10-Ubuntu-0ubuntu0.20.04.1
+1 💚	javadoc	0m 25s	the patch passed with JDK Private Build-1.8.0_312-8u312-b07-0ubuntu1~20.04-b07
+1 💚	findbugs	1m 2s	the patch passed
		_ Other Tests _
+1 💚	unit	2m 5s	tez-api in the patch passed.
+1 💚	asflicense	0m 15s	The patch does not generate ASF License warnings.
		30m 28s

Subsystem	Report/Notes
Docker	ClientAPI=1.41 ServerAPI=1.41 base: https://ci-hadoop.apache.org/job/tez-multibranch/job/PR-209/3/artifact/out/Dockerfile
GITHUB PR	#209
Optional Tests	dupname asflicense javac javadoc unit spotbugs findbugs checkstyle compile
uname	Linux e757c91223e4 4.15.0-175-generic #184-Ubuntu SMP Thu Mar 24 17:48:36 UTC 2022 x86_64 x86_64 x86_64 GNU/Linux
Build tool	maven
Personality	personality/tez.sh
git revision	master / `798ddda`
Default Java	Private Build-1.8.0_312-8u312-b07-0ubuntu1~20.04-b07
Multi-JDK versions	/usr/lib/jvm/java-11-openjdk-amd64:Private Build-11.0.15+10-Ubuntu-0ubuntu0.20.04.1 /usr/lib/jvm/java-8-openjdk-amd64:Private Build-1.8.0_312-8u312-b07-0ubuntu1~20.04-b07
checkstyle	https://ci-hadoop.apache.org/job/tez-multibranch/job/PR-209/3/artifact/out/diff-checkstyle-tez-api.txt
Test Results	https://ci-hadoop.apache.org/job/tez-multibranch/job/PR-209/3/testReport/
Max. process+thread count	398 (vs. ulimit of 5500)
modules	C: tez-api U: tez-api
Console output	https://ci-hadoop.apache.org/job/tez-multibranch/job/PR-209/3/console
versions	git=2.25.1 maven=3.6.3 findbugs=3.0.1
Powered by	Apache Yetus 0.12.0 https://yetus.apache.org

This message was automatically generated.

abstractdog · 2022-05-07T12:17:38Z

thanks @skysiders , there is 1 more checkstyle issue reported

skysiders · 2022-05-07T12:37:15Z

Thanks for your review @abstractdog , I use checkstyle:check find issue in import. I used the new FsPermission(700) check as assert at first, but then I found it would be more appropriate to use TezCommonUtils.TEZ_AM_DIR_PERMISSION, so I replaced the content in the assert, and forgot to delete the import.

tez-yetus · 2022-05-07T13:00:02Z

🎊 +1 overall

Vote	Subsystem	Runtime	Comment
+0 🆗	reexec	0m 59s	Docker mode activated.
		_ Prechecks _
+1 💚	dupname	0m 0s	No case conflicting files found.
+1 💚	@author	0m 0s	The patch does not contain any @author tags.
+1 💚	test4tests	0m 0s	The patch appears to include 1 new or modified test files.
		_ master Compile Tests _
+1 💚	mvninstall	16m 11s	master passed
+1 💚	compile	0m 45s	master passed with JDK Private Build-11.0.15+10-Ubuntu-0ubuntu0.20.04.1
+1 💚	compile	0m 41s	master passed with JDK Private Build-1.8.0_312-8u312-b07-0ubuntu1~20.04-b07
+1 💚	checkstyle	1m 9s	master passed
+1 💚	javadoc	0m 56s	master passed with JDK Private Build-11.0.15+10-Ubuntu-0ubuntu0.20.04.1
+1 💚	javadoc	0m 44s	master passed with JDK Private Build-1.8.0_312-8u312-b07-0ubuntu1~20.04-b07
+0 🆗	spotbugs	1m 38s	Used deprecated FindBugs config; considering switching to SpotBugs.
+1 💚	findbugs	1m 36s	master passed
		_ Patch Compile Tests _
+1 💚	mvninstall	0m 23s	the patch passed
+1 💚	compile	0m 26s	the patch passed with JDK Private Build-11.0.15+10-Ubuntu-0ubuntu0.20.04.1
+1 💚	javac	0m 26s	the patch passed
+1 💚	compile	0m 22s	the patch passed with JDK Private Build-1.8.0_312-8u312-b07-0ubuntu1~20.04-b07
+1 💚	javac	0m 22s	the patch passed
+1 💚	checkstyle	0m 13s	the patch passed
+1 💚	whitespace	0m 0s	The patch has no whitespace issues.
+1 💚	javadoc	0m 25s	the patch passed with JDK Private Build-11.0.15+10-Ubuntu-0ubuntu0.20.04.1
+1 💚	javadoc	0m 25s	the patch passed with JDK Private Build-1.8.0_312-8u312-b07-0ubuntu1~20.04-b07
+1 💚	findbugs	1m 2s	the patch passed
		_ Other Tests _
+1 💚	unit	2m 6s	tez-api in the patch passed.
+1 💚	asflicense	0m 16s	The patch does not generate ASF License warnings.
		29m 32s

Subsystem	Report/Notes
Docker	ClientAPI=1.41 ServerAPI=1.41 base: https://ci-hadoop.apache.org/job/tez-multibranch/job/PR-209/4/artifact/out/Dockerfile
GITHUB PR	#209
Optional Tests	dupname asflicense javac javadoc unit spotbugs findbugs checkstyle compile
uname	Linux e045ab3e49ea 4.15.0-175-generic #184-Ubuntu SMP Thu Mar 24 17:48:36 UTC 2022 x86_64 x86_64 x86_64 GNU/Linux
Build tool	maven
Personality	personality/tez.sh
git revision	master / `798ddda`
Default Java	Private Build-1.8.0_312-8u312-b07-0ubuntu1~20.04-b07
Multi-JDK versions	/usr/lib/jvm/java-11-openjdk-amd64:Private Build-11.0.15+10-Ubuntu-0ubuntu0.20.04.1 /usr/lib/jvm/java-8-openjdk-amd64:Private Build-1.8.0_312-8u312-b07-0ubuntu1~20.04-b07
Test Results	https://ci-hadoop.apache.org/job/tez-multibranch/job/PR-209/4/testReport/
Max. process+thread count	391 (vs. ulimit of 5500)
modules	C: tez-api U: tez-api
Console output	https://ci-hadoop.apache.org/job/tez-multibranch/job/PR-209/4/console
versions	git=2.25.1 maven=3.6.3 findbugs=3.0.1
Powered by	Apache Yetus 0.12.0 https://yetus.apache.org

This message was automatically generated.

abstractdog · 2022-05-09T06:24:58Z

+ Configuration remoteConf = new Configuration();
+ remoteConf.set(MiniDFSCluster.HDFS_MINIDFS_BASEDIR, TEST_ROOT_DIR);
+ remoteConf.set(CommonConfigurationKeys.FS_PERMISSIONS_UMASK_KEY, "777");
+ MiniDFSCluster miniDFS = new MiniDFSCluster.Builder(remoteConf).numDataNodes(3).format(true).racks(null)


unit test works as expected, this patch is so close, 1 more thing, could you please shutdown miniDFS cluster?

Hi @abstractdog .Thanks for your suggestion, In this patch, I closed miniDFS after I ran out of it. Thank you for reviewing my code, it has taught me a lot.

merged to master, thanks @skysiders for this patch, your tireless work, and quick responses!

tez-yetus · 2022-05-09T07:19:20Z

🎊 +1 overall

Vote	Subsystem	Runtime	Comment
+0 🆗	reexec	1m 2s	Docker mode activated.
		_ Prechecks _
+1 💚	dupname	0m 0s	No case conflicting files found.
+1 💚	@author	0m 0s	The patch does not contain any @author tags.
+1 💚	test4tests	0m 0s	The patch appears to include 1 new or modified test files.
		_ master Compile Tests _
+1 💚	mvninstall	15m 58s	master passed
+1 💚	compile	0m 46s	master passed with JDK Private Build-11.0.15+10-Ubuntu-0ubuntu0.20.04.1
+1 💚	compile	0m 42s	master passed with JDK Private Build-1.8.0_312-8u312-b07-0ubuntu1~20.04-b07
+1 💚	checkstyle	1m 8s	master passed
+1 💚	javadoc	0m 56s	master passed with JDK Private Build-11.0.15+10-Ubuntu-0ubuntu0.20.04.1
+1 💚	javadoc	0m 44s	master passed with JDK Private Build-1.8.0_312-8u312-b07-0ubuntu1~20.04-b07
+0 🆗	spotbugs	1m 39s	Used deprecated FindBugs config; considering switching to SpotBugs.
+1 💚	findbugs	1m 36s	master passed
		_ Patch Compile Tests _
+1 💚	mvninstall	0m 23s	the patch passed
+1 💚	compile	0m 27s	the patch passed with JDK Private Build-11.0.15+10-Ubuntu-0ubuntu0.20.04.1
+1 💚	javac	0m 27s	the patch passed
+1 💚	compile	0m 23s	the patch passed with JDK Private Build-1.8.0_312-8u312-b07-0ubuntu1~20.04-b07
+1 💚	javac	0m 23s	the patch passed
+1 💚	checkstyle	0m 14s	the patch passed
+1 💚	whitespace	0m 0s	The patch has no whitespace issues.
+1 💚	javadoc	0m 26s	the patch passed with JDK Private Build-11.0.15+10-Ubuntu-0ubuntu0.20.04.1
+1 💚	javadoc	0m 25s	the patch passed with JDK Private Build-1.8.0_312-8u312-b07-0ubuntu1~20.04-b07
+1 💚	findbugs	1m 2s	the patch passed
		_ Other Tests _
+1 💚	unit	2m 6s	tez-api in the patch passed.
+1 💚	asflicense	0m 14s	The patch does not generate ASF License warnings.
		29m 24s

Subsystem	Report/Notes
Docker	ClientAPI=1.41 ServerAPI=1.41 base: https://ci-hadoop.apache.org/job/tez-multibranch/job/PR-209/5/artifact/out/Dockerfile
GITHUB PR	#209
Optional Tests	dupname asflicense javac javadoc unit spotbugs findbugs checkstyle compile
uname	Linux 0801a38611fa 4.15.0-175-generic #184-Ubuntu SMP Thu Mar 24 17:48:36 UTC 2022 x86_64 x86_64 x86_64 GNU/Linux
Build tool	maven
Personality	personality/tez.sh
git revision	master / `798ddda`
Default Java	Private Build-1.8.0_312-8u312-b07-0ubuntu1~20.04-b07
Multi-JDK versions	/usr/lib/jvm/java-11-openjdk-amd64:Private Build-11.0.15+10-Ubuntu-0ubuntu0.20.04.1 /usr/lib/jvm/java-8-openjdk-amd64:Private Build-1.8.0_312-8u312-b07-0ubuntu1~20.04-b07
Test Results	https://ci-hadoop.apache.org/job/tez-multibranch/job/PR-209/5/testReport/
Max. process+thread count	423 (vs. ulimit of 5500)
modules	C: tez-api U: tez-api
Console output	https://ci-hadoop.apache.org/job/tez-multibranch/job/PR-209/5/console
versions	git=2.25.1 maven=3.6.3 findbugs=3.0.1
Powered by	Apache Yetus 0.12.0 https://yetus.apache.org

This message was automatically generated.

…209) (Zhang Dongsheng reviewed by Laszlo Bodor)

ensure mkDirForAM create directory with special permissions
7b42513

This comment was marked as outdated.
Sign in to view

abstractdog self-requested a review May 7, 2022 08:00

abstractdog requested changes May 7, 2022
View reviewed changes

add UnitTest and fix checkstyle
7280852

skysiders requested a review from abstractdog May 7, 2022 08:31

This comment was marked as outdated.
Sign in to view

fix mistake in UnitTest
e1f8374

fix mistake in UnitTest
85039e3

abstractdog reviewed May 9, 2022
View reviewed changes

shutdown miniDFS cluster
af4f8d1

skysiders requested a review from abstractdog May 9, 2022 06:52

abstractdog approved these changes May 10, 2022
View reviewed changes

abstractdog merged commit 98a81b1 into apache:masterMay 10, 2022

asfgit pushed a commit that referenced this pull request May 10, 2022
TEZ-4412 ensure mkDirForAM create directory with special permissions (#…
8e3a23b
…209) (Zhang Dongsheng reviewed by Laszlo Bodor)

skysiders mentioned this pull request May 10, 2022
HIVE-26201 QueryResultsCache should add permission check after mkdir with special permission apache/hive#3267
Closed

skysiders mentioned this pull request Jan 5, 2023
HIVE-26887 Make sure dirPath has the correct permissions apache/hive#3894
Closed

Uh oh!

Conversation

skysiders commented May 3, 2022

Uh oh!

This comment was marked as outdated.

skysiders commented May 5, 2022

Uh oh!

abstractdog commented May 7, 2022

Uh oh!

abstractdogMay 7, 2022

Choose a reason for hiding this comment

Uh oh!

skysidersMay 7, 2022

Choose a reason for hiding this comment

Uh oh!

skysidersMay 7, 2022

Choose a reason for hiding this comment

Uh oh!

This comment was marked as outdated.

skysiders commented May 7, 2022

Uh oh!

tez-yetus commented May 7, 2022

Uh oh!

abstractdog commented May 7, 2022

Uh oh!

skysiders commented May 7, 2022

Uh oh!

tez-yetus commented May 7, 2022

Uh oh!

abstractdogMay 9, 2022

Choose a reason for hiding this comment

Uh oh!

skysidersMay 9, 2022

Choose a reason for hiding this comment

Uh oh!

abstractdogMay 10, 2022

Choose a reason for hiding this comment

Uh oh!

tez-yetus commented May 9, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants