HADOOP-12774 use UGI.currentUser for user and group of s3a objects #136
Conversation
There was a problem hiding this comment.
@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.
@steveloughran , here are a few more comments on the patch. Thank you.
| @@ -32,18 +32,24 @@ | |||
| @InterfaceStability.Evolving | |||
| public class S3AFileStatus extends FileStatus { | |||
| private boolean isEmptyDirectory; | |||
| private final String owner; | |||
There was a problem hiding this comment.
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.
| super(0, isdir, 1, 0, 0, path); | ||
| isEmptyDirectory = isemptydir; | ||
| this.owner = owner; |
There was a problem hiding this comment.
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.)
| @@ -52,7 +58,16 @@ public boolean isEmptyDirectory() { | |||
|
|
|||
| @Override | |||
| public String getOwner() { | |||
There was a problem hiding this comment.
If we implement the above comments, then we can completely remove the override of getOwner here.
| * @return the owner. | ||
| */ | ||
| @Override | ||
| public String getGroup() { |
There was a problem hiding this comment.
The override of getGroup could be removed too.
| @@ -42,8 +42,11 @@ | |||
|
|
|||
| import java.io.File; | |||
| import java.net.URI; | |||
| import java.security.PrivilegedAction; | |||
| super(length, false, 1, blockSize, modification_time, path); | ||
| isEmptyDirectory = false; | ||
| this.owner = owner; |
There was a problem hiding this comment.
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 <jvenkatr@linkedin.com> Reviewers: Prateek Maheshwari<prateekm@linkedin.com> 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.