-
Couldn't load subscription status.
- Fork 9.1k
HADOOP-12774 use UGI.currentUser for user and group of s3a objects #136
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.
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
HADOOP-12774 use UGI.currentUser for user and group of s3a objects #136
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@steveloughran , could you please add a test that initializes an S3AFileSystem as a specific user, such as by using UserGroupInformation#createUserForTesting and UserGroupInformation#doAs? The specific test username can be a hard-coded string that is likely to be different from the user.name system property.
|
Patch 002: create a fake user, create an FS and verify that the user and owner on the root path is that username |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@steveloughran , here are a few more comments on the patch. Thank you.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we can achieve this change without adding a member variable in the subclass. (See more specific notes to follow.) Removing this member variable would reduce memory footprint. Admittedly, the memory cost is probably not significant, but as we start thinking about the possibility of caching FileStatus instances client-side for things like S3Guard, then the per-instance memory cost of each FileStatus could become more significant.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider removing the member variable and changing this line of code to:
this.setOwner(owner);
this.setGroup(owner);
(These are protected methods in the base class.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we implement the above comments, then we can completely remove the override of getOwner here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The override of getGroup could be removed too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unused import?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Similar to the earlier comment:
this.setOwner(owner);
this.setGroup(owner);
2c06cc5 to
29e589c
Compare
29e589c to
a06b635
Compare
…and owner on the root path is that username
…s. Also, given that the first contructor for S3AFileStatus is only for directories, the isdir parameter is always true, hence superflous. Cut and add javadocs for both constructors.
a06b635 to
fae8ee7
Compare
…istic behavior Author: vjagadish1989 <[email protected]> Reviewers: Prateek Maheshwari<[email protected]> Closes apache#136 from vjagadish1989/samza-1202
This patch grabs the UGI current user shortname in the FS initialize call, then uses that as the user and group for all filestatus instances generated.