HADOOP-17029. Return correct permission and owner for listing on internal directories in ViewFs #2019
Conversation
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
| result[i++] = new FileStatus(0, false, 0, 0, | ||
| creationTime, creationTime, PERMISSION_555, | ||
| ugi.getShortUserName(), ugi.getPrimaryGroupName(), | ||
| link.getTargetLink(), | ||
| new Path(inode.fullPath).makeQualified( | ||
| myUri, null)); |
There was a problem hiding this comment.
two comments:
- seems to be some duplicate code, the "else" branch is pretty much the same, can we refactor here?
- Not sure if this is the best way when dealing with FileNotFoundException. If I understand this correctly, it is possible that some mounts does not have this path, so it can hit FileNotFoundException?
If this is the case, I wonder if it makes more sense to just skip this mount, by not adding a FileStatus for mount at all. So that clients do not get confused by an actually non-existing FileStatus, among other existing ones. But one issue here would be that result array is strictly the size of the # of mounts. Creating result as a list, append, and then return as a array may resolve this.
There was a problem hiding this comment.
For # 2: The problem here I think we may not be able reset mount automatically, so until some one checks file exists or do op on target, we will not know whether the file exists or not. This will continue until user updates mount points accordingly.
This can be possible when some one deletes the target directory directly but not updated the mount tables accordingly. Please check one of my comment on behavior of ls in MAC. Also we have other issue: isDir is inconsistent.
There was a problem hiding this comment.
Sorry if I am missing anything. The change with catching the FileNotFoundException is good ? or I need to make a change.
There was a problem hiding this comment.
Sorry if I am missing anything. The change with catching the FileNotFoundException is good ? or I need to change something. @umamaheswararao
There was a problem hiding this comment.
IMO, FileNotFoundException is fine. Please see my comment below https://github.com/apache/hadoop/pull/2019/files#r430611396
@chliang71 , What do you say?
| inodelink.getTargetLink(), | ||
| new Path(inode.fullPath).makeQualified( | ||
| myUri, null)); | ||
| } catch (FileNotFoundException ex) { |
There was a problem hiding this comment.
similar comment regarding FileNotFoundException. I think in general, it's better to match behavior of non-federated client. If a path does not exist, just throw back FileNotFoundException.
There was a problem hiding this comment.
similar comment regarding FileNotFoundException. I think in general, it's better to match behavior of non-federated client. If a path does not exist, just throw back FileNotFoundException.
I just verified symlinks. When target deleted, ls on symlink does not throw FNFE. Instead it is converted to file link. I tested dir->dir link.
It seems this behavior is correct when compared with other fs. I tested on my MAC.
Should this be fixed in federated clusters is necessary ? Could you please validate this?
If we attempt to open that non existent link file, then we can throw out exception. But ls seems to simply pass.
Work % mkdir linkTarget
Work % ln -s linkTarget linkSrc
Work % ls -l
lrwxr-xr-x 1 umagangumalla xxxx 10 May 26 11:08 linkSrc -> linkTarget
Work % rm -rf linkTarget
Work % ls -l
lrwxr-xr-x 1 umagangumalla xxxx 10 May 26 11:08 linkSrc -> linkTarget
Work % cd linkSrc
cd: no such file or directory: linkSrc
| link.getTargetLink(), | ||
| new Path(inode.fullPath).makeQualified( | ||
| myUri, null)); | ||
| } catch (FileNotFoundException ex) { |
There was a problem hiding this comment.
same comment about FileNotFoundException here
| } | ||
|
|
||
| @Test | ||
| public void testListStatusACL() |
There was a problem hiding this comment.
some javadoc, and comments in the code could be helpful
There was a problem hiding this comment.
Thank you @abhishekdas99 for working on this.
Thanks @chliang71 for reviews. In addition to @chliang71 review, I just added few comments. Please take a look. Thanks
| // For MERGE or NFLY links, the first target link is considered | ||
| // for fetching the FileStatus with an assumption that the permission | ||
| // and the owner will be the same for all the target directories. | ||
| Path linkedPath = new Path(link.targetDirLinkList[0].toString()); |
There was a problem hiding this comment.
instead of link.targetDirLinkList[0], link.getTargetFileSystem().getUri() should work?
This might work for nfly also I think. Because nfly has its own GetFileStatus impl, probably we should use that impl only instead of getting one targetDirLinkList[0]
There was a problem hiding this comment.
In the new change , I have getting the filesystem by link.getTargetFileSystem() and calling getFileStatus on slash path.
| inodelink.getTargetLink(), | ||
| new Path(inode.fullPath).makeQualified( | ||
| myUri, null)); | ||
| } catch (FileNotFoundException ex) { |
There was a problem hiding this comment.
similar comment regarding FileNotFoundException. I think in general, it's better to match behavior of non-federated client. If a path does not exist, just throw back FileNotFoundException.
I just verified symlinks. When target deleted, ls on symlink does not throw FNFE. Instead it is converted to file link. I tested dir->dir link.
It seems this behavior is correct when compared with other fs. I tested on my MAC.
Should this be fixed in federated clusters is necessary ? Could you please validate this?
If we attempt to open that non existent link file, then we can throw out exception. But ls seems to simply pass.
Work % mkdir linkTarget
Work % ln -s linkTarget linkSrc
Work % ls -l
lrwxr-xr-x 1 umagangumalla xxxx 10 May 26 11:08 linkSrc -> linkTarget
Work % rm -rf linkTarget
Work % ls -l
lrwxr-xr-x 1 umagangumalla xxxx 10 May 26 11:08 linkSrc -> linkTarget
Work % cd linkSrc
cd: no such file or directory: linkSrc
...on-project/hadoop-common/src/test/java/org/apache/hadoop/fs/viewfs/TestViewfsFileStatus.java
Show resolved
Hide resolved
| result[i++] = new FileStatus(0, false, 0, 0, | ||
| creationTime, creationTime, PERMISSION_555, | ||
| ugi.getShortUserName(), ugi.getPrimaryGroupName(), | ||
| link.getTargetLink(), | ||
| new Path(inode.fullPath).makeQualified( | ||
| myUri, null)); |
There was a problem hiding this comment.
For # 2: The problem here I think we may not be able reset mount automatically, so until some one checks file exists or do op on target, we will not know whether the file exists or not. This will continue until user updates mount points accordingly.
This can be possible when some one deletes the target directory directly but not updated the mount tables accordingly. Please check one of my comment on behavior of ls in MAC. Also we have other issue: isDir is inconsistent.
|
💔 -1 overall
This message was automatically generated. |
abhishekdas99
force-pushed
the
HADOOP-17029
branch
2 times, most recently
from
cf990e0 to
49071eb
Compare
|
💔 -1 overall
This message was automatically generated. |
abhishekdas99
force-pushed
the
HADOOP-17029
branch
from
49071eb to
da1702a
Compare
|
💔 -1 overall
This message was automatically generated. |
| result[i++] = new FileStatus(0, false, 0, 0, | ||
| creationTime, creationTime, PERMISSION_555, | ||
| ugi.getShortUserName(), ugi.getPrimaryGroupName(), | ||
| link.getTargetLink(), | ||
| new Path(inode.fullPath).makeQualified( | ||
| myUri, null)); |
There was a problem hiding this comment.
IMO, FileNotFoundException is fine. Please see my comment below https://github.com/apache/hadoop/pull/2019/files#r430611396
@chliang71 , What do you say?
| myUri, null)); | ||
| try { | ||
| FileStatus status = link.getTargetFileSystem() | ||
| .getFileStatus(new Path("/")); |
There was a problem hiding this comment.
Why are we getting status on "/"? In practical scenario it should work. However what if the target uri is a file?
Should we simply use link.getTargetFileSystem().getUri() ?
Could you please check scenario? If this works, I have no other changes.
Thanks for update.
There was a problem hiding this comment.
Changed to link.getTargetFileSystem().getUri(). Used ChRootedFileSystem to obtain the filestatus. ((ChRootedFileSystem)link.getTargetFileSystem()).getMyFs().getFileStatus().
|
💔 -1 overall
This message was automatically generated. |
|
+1 Looks good to me. Thanks for working on it. If no other objections, I will commit it in some time. |
Thanks @umamaheswararao. Sorry for the delay |
HADOOP-17029. ViewFS does not return correct user/group and ACL