HADOOP-18122. ViewFileSystem fails on determining owning group when primary group doesn't exist for user #3987
+184
−30
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
ayushtkn
reviewed
Feb 11, 2022
There was a problem hiding this comment.
just had a cursory look. This I don't think will fix the bug, but will just give a workaround like, if you don't have a primaryGroup get this config set and you can dodge it?
Why didn't we try something like:
https://github.com/apache/hadoop/blob/trunk/hadoop-hdfs-project/hadoop-hdfs-rbf/src/main/java/org/apache/hadoop/hdfs/server/federation/store/records/MountTable.java#L152-L153
Secondly, Why is user name also made configurable?
Thirdly, Just saw the commit:
virajith authored and cheyu2022 committed
seems you are using some wrong author?
fyi. @virajith
...p-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/viewfs/ViewFileSystem.java
Outdated
Show resolved
Hide resolved
...p-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/viewfs/ViewFileSystem.java
Outdated
Show resolved
Hide resolved
...p-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/viewfs/ViewFileSystem.java
Outdated
Show resolved
Hide resolved
...-project/hadoop-common/src/test/java/org/apache/hadoop/fs/viewfs/ViewFileSystemBaseTest.java
Outdated
Show resolved
Hide resolved
...-project/hadoop-common/src/test/java/org/apache/hadoop/fs/viewfs/ViewFileSystemBaseTest.java
Outdated
Show resolved
Hide resolved
You are right about this. This fix is more like a workaround. But even this solution above sounds like a workaround as well - it just assumes if group isn't found, use username. I'm ok with either way.
Mount points can essentially have any user names, thus make it configurable as well.
Yeah nice catch, I will fix that. |
cheyu2022
force-pushed
the
HADOOP-18122
branch
from
2e67eb4 to
8aabc9e
Compare
|
🎊 +1 overall
This message was automatically generated. |
ayushtkn
reviewed
Feb 15, 2022
...p-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/viewfs/ViewFileSystem.java
Outdated
Show resolved
Hide resolved
cheyu2022
force-pushed
the
HADOOP-18122
branch
from
8aabc9e to
be45af5
Compare
|
🎊 +1 overall
This message was automatically generated. |
ayushtkn
reviewed
Feb 23, 2022
...-project/hadoop-common/src/test/java/org/apache/hadoop/fs/viewfs/ViewFileSystemBaseTest.java
Outdated
Show resolved
Hide resolved
cheyu2022
force-pushed
the
HADOOP-18122
branch
from
be45af5 to
f8954a2
Compare
|
🎊 +1 overall
This message was automatically generated. |
|
Kindly ask for a +1 and ship this @ayushtkn |
virajith
reviewed
Apr 15, 2022
There was a problem hiding this comment.
Instead of setting this, a cleaner way is to declare mountLinkGroupName as a final variable and return ugi.getPrimaryGroupName() here. Same goes for the implementation of getMountLinkUserName()
There was a problem hiding this comment.
alternatively, just set mountLinkGroupName to ugi.getPrimaryGroupName() in the constructor if the config is not set?
There was a problem hiding this comment.
Go with the second option, updated.
There was a problem hiding this comment.
Tip for future - leave the comment unresolved so that the person who asked for the change can resolve it. It makes it easier for the reviewer to see what changed.
There was a problem hiding this comment.
Actually, for the second option we changed the behavior by setting mountLinkGroupName to ugi.getPrimaryGroupName() in the constructor, since before we only set it on demand when we call listStatus(), getFileStatus(), etc... Not sure this behavior change will cause other problems but this failed some unit tests like testListStatusWithNoGroups with error "There is no primary group for UGI" when we initialize InternalDirOfViewFs. Because in the test we don't expect the error during filesystem initialization.
There was a problem hiding this comment.
For your first suggested option:
I think the idea of setting mountLinkGroupName in this function is to not call ugi.getPrimaryGroupName() every time we call this get method. We set mountLinkGroupName once if it's null then there's no need to call ugi.getPrimaryGroupName() again.
…rimary group doesn't exist for user
virajith
requested changes
Apr 20, 2022
| } | ||
|
|
||
| private FsPermission getMountLinkDefaultPermissions() { | ||
| return PERMISSION_555; |
There was a problem hiding this comment.
Do you want to make this configurable as well?
| final UserGroupInformation currentUser = | ||
| UserGroupInformation.getCurrentUser(); | ||
| FileStatus status = fs.getFileStatus(new Path("/internalDir")); | ||
| assertEquals(currentUser.getUserName(), status.getOwner()); |
There was a problem hiding this comment.
currentUser.getShortUserName()?
| UserGroupInformation.getCurrentUser(); | ||
| FileStatus status = fs.getFileStatus(new Path("/internalDir")); | ||
| assertEquals(currentUser.getUserName(), status.getOwner()); | ||
| assertEquals(currentUser.getGroupNames()[0], status.getGroup()); |
There was a problem hiding this comment.
currentUser.getPrimaryGroupName()?
| final UserGroupInformation currentUser = | ||
| UserGroupInformation.getCurrentUser(); | ||
| FileStatus status = fc.getFileStatus(new Path("/internalDir")); | ||
| assertEquals(currentUser.getUserName(), status.getOwner()); |
There was a problem hiding this comment.
currentUser.getShortUserName()?
| UserGroupInformation.getCurrentUser(); | ||
| FileStatus status = fc.getFileStatus(new Path("/internalDir")); | ||
| assertEquals(currentUser.getUserName(), status.getOwner()); | ||
| assertEquals(currentUser.getGroupNames()[0], status.getGroup()); |
There was a problem hiding this comment.
currentUser.getPrimaryGroupName()?
| myUri = uri; | ||
| this.fsState = fsState; | ||
| this.conf = conf; | ||
| mountLinkUserName = conf.get(CONFIG_VIEWFS_MOUNT_LINKS_USER_NAME); |
There was a problem hiding this comment.
may be write this as conf.get(CONFIG_VIEWFS_MOUNT_LINKS_USER_NAME) == null ? ugi.getShortUserName() : conf.get(CONFIG_VIEWFS_MOUNT_LINKS_USER_NAME) and explain why you are not using conf.get(CONFIG_VIEWFS_MOUNT_LINKS_USER_NAME, ugi.getShortUserName()) in a comment? Otherwise, it's unclear why you aren't using that method directly. Same for the other places where you are using this template
|
💔 -1 overall
This message was automatically generated. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Description of PR
ViewFileSystem should not fail on determining owning group when primary group doesn't exist for user
How was this patch tested?
new unit test; run existing unit tests
mvn test -Dtest=TestViewFileSystem*For code changes:
LICENSE,LICENSE-binary,NOTICE-binaryfiles?