Skip to content

[SPARK-41943][CORE] Use java api to create files and grant permissions #39448

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Closed
wants to merge 3 commits into from
Closed

[SPARK-41943][CORE] Use java api to create files and grant permissions #39448

wants to merge 3 commits into from

Conversation

@smallzhongfeng
Copy link
Contributor

@smallzhongfeng smallzhongfeng commented Jan 7, 2023

What changes were proposed in this pull request?

For method createDirWithPermission770, using java api to create files and grant permissions instead of calling shell commands.

Why are the changes needed?

Safer and more efficient.

Does this PR introduce any user-facing change?

No.

How was this patch tested?

Origin uts.

@github-actions github-actions bot added the CORE label Jan 7, 2023
smallzhongfeng
smallzhongfeng commented Jan 7, 2023
View reviewed changes
core/src/main/scala/org/apache/spark/storage/DiskBlockManager.scala
* Create a directory that is writable by the group.
* Grant the permission 770 "rwxrwx---" to the directory so the shuffle server can
* create subdirs/files within the merge folder.
* TODO: Find out why can't we create a dir using java api with permission 770
Copy link
Contributor Author

@smallzhongfeng smallzhongfeng Jan 7, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For TODO, maybe we can see https://bugs.openjdk.org/browse/JDK-8220013

Copy link
Member

@HyukjinKwon HyukjinKwon Jan 9, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mind summarizing why we can use this approach while the JDK issue isn't resolved?

Copy link
Contributor Author

@smallzhongfeng smallzhongfeng Jan 9, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In unix like systems every process has a property called umask which is masked onto the permissions of any file created and inherited by child processes. the default is 0002 or "turn off write for others". So it's most likely that Java is setting the permission you seek and then it's being masked out. At present, this issue has not been resolved, so my current approach is to create a directory first, and then explicitly grant permissions.

@mridulm
Copy link
Contributor

mridulm commented Jan 8, 2023

+CC @zhouyejoe

@smallzhongfeng
Copy link
Contributor Author

smallzhongfeng commented Jan 8, 2023

cc @cloud-fan @HyukjinKwon @LuciferYang @@zhouyejoe Hope to get your opinion. :)

@smallzhongfeng smallzhongfeng changed the title [CORE] Use java api to create files and grant permissions SPARK-41943[CORE] Use java api to create files and grant permissions Jan 8, 2023
@smallzhongfeng smallzhongfeng changed the title SPARK-41943[CORE] Use java api to create files and grant permissions [SPARK-41943][CORE] Use java api to create files and grant permissions Jan 8, 2023
@zhouyejoe
Copy link
Contributor

zhouyejoe commented Jan 9, 2023

There was a long discussion thread regarding this implementation in this PR. There will be some issue with setgid.
Will add @otterc @akpatnam25 @Kimahriman for additional comments and review.

@smallzhongfeng
Copy link
Contributor Author

smallzhongfeng commented Jan 9, 2023

There was a long discussion thread regarding this implementation in this PR. There will be some issue with setgid. Will add @otterc @akpatnam25 @Kimahriman for additional comments and review.

Thank you for reminding me @zhouyejoe . I looked at the results of the discussion and it seems to be consistent with my handling method. You can see #35085 (comment)

@Kimahriman
Copy link
Contributor

Kimahriman commented Jan 9, 2023 •
edited
Loading

Yeah I think this is fine. mkdir -p essentially does a mkdir syscall followed by a chmod sys call, which is effectively what this is doing as well. Both approaches get around the umask but lose the setgid bit which is why all the make shuffle files world readable was necessary in #35085

@smallzhongfeng
Copy link
Contributor Author

smallzhongfeng commented Jan 9, 2023

Could you take another look for this? @mridulm @zhouyejoe @HyukjinKwon @Kimahriman @cloud-fan THX!

@AmplabJenkins
Copy link

AmplabJenkins commented Jan 9, 2023

Can one of the admins verify this patch?

@cloud-fan
Copy link
Contributor

cloud-fan commented Jan 10, 2023

thanks, merging to master!

@cloud-fan cloud-fan closed this in b22946e Jan 10, 2023
@smallzhongfeng
Copy link
Contributor Author

smallzhongfeng commented Jan 10, 2023

Thanks all. @mridulm @zhouyejoe @Kimahriman @HyukjinKwon @cloud-fan

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@HyukjinKwon HyukjinKwon HyukjinKwon left review comments

@cloud-fan cloud-fan cloud-fan approved these changes

+1 more reviewer

@zhouyejoe zhouyejoe zhouyejoe approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

CORE

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

@smallzhongfeng @mridulm @zhouyejoe @Kimahriman @AmplabJenkins @cloud-fan @HyukjinKwon