Details
-
New Feature
-
Status: Resolved
-
Major
-
Resolution: Fixed
-
None
-
None
-
None
-
None
-
Incompatible change, Reviewed
-
HideAdds Symbolic links to FileContext, AbstractFileSystem.
It also adds a limited implementation for the local file system (RawLocalFs) that allows local symlinks.
Breaks compatibility: FileStatus#isDir() if used to determine if the object is a file will be incorrect if the object happens to be a symlink.ShowAdds Symbolic links to FileContext, AbstractFileSystem. It also adds a limited implementation for the local file system (RawLocalFs) that allows local symlinks. Breaks compatibility: FileStatus#isDir() if used to determine if the object is a file will be incorrect if the object happens to be a symlink.
Description
Here's a jira for the common parts of HDFS-245, mostly changes to FileContext and AbstractFileSystem.
Attachments
Attachments
- symlink24-common.patch
- 34 kB
- Eli Collins
- symlink-25-common.patch
- 48 kB
- Eli Collins
- symlink-26-common.patch
- 48 kB
- Eli Collins
- symlink-26-common.patch
- 49 kB
- Eli Collins
- symlink27-common.patch
- 59 kB
- Eli Collins
- symlink28-common.patch
- 48 kB
- Eli Collins
- symlink29-common.patch
- 50 kB
- Eli Collins
- symlink29-common.patch
- 50 kB
- Eli Collins
- symlink29-common.patch
- 50 kB
- Eli Collins
- symlink30-common.patch
- 51 kB
- Eli Collins
- symlink31-common.patch
- 74 kB
- Eli Collins
- symlink32-common.patch
- 74 kB
- Eli Collins
- symlink33-common.patch
- 75 kB
- Eli Collins
- symlink34-common.patch
- 79 kB
- Eli Collins
- symlink35-common.patch
- 95 kB
- Eli Collins
- symlink36-common.patch
- 92 kB
- Eli Collins
- symlink37-common.patch
- 92 kB
- Eli Collins
- symlink38-common.patch
- 97 kB
- Eli Collins
- symlink39-common.patch
- 97 kB
- Eli Collins
Issue Links
- is blocked by
-
HADOOP-6427 Add Path isQualified
- Resolved
- is depended upon by
-
HDFS-245 Create symbolic links in HDFS
- Closed
- relates to
-
HDFS-245 Create symbolic links in HDFS
- Closed
-
HADOOP-6585 Add FileStatus#isDirectory and isFile
- Closed
Activity
Should we add an implementation to LocalFileSystem here? That would enable us to add some tests. It could use shell commands to implement createSymlink() and getLinkTarget(). The hardest bit might be implementing listStatus() and getStatus() to correctly support isLink().
Testing could instead be accomplished with a FilterFileSystem implementation used only for testing. It would be nice to have a usable implemenation for the local fs, but that could be left to a separate issue.
I'm actually working on adding them to LocalFs today. Do you think we need LocalFileSystem as well? I uploaded a test plan to HDFS-254 yesterday, the things marked xxx are what I'm currently working on that block further testing (like LocalFs).
Attached the latest patch, includes a symlink implementation for LocalFs and more tests.
By the way, I ended up shelling out to implement createSymlink, getLinkTarget and getFileLinkStatus. I also wrote an implementation that avoids this by making a NativeCall class that makes jni calls to link, readlink etc but integrating it into the native build was a pain. Does anyone know why all the auto-generated build files are checked in to src/native?
> Does anyone know why all the auto-generated build files are checked in to src/native?
I think it's so that developers don't have to have automake etc. installed. Seems like a strange optimization though. Owen?
It isn't an optimization as much as making sure more people can compile the system.
At this point, most platforms that we care about, it is easy to install autoconf and automake. If someone wants to open a jira that deletes the files (and moves them into build!) that would be great. It would make the auto line counting tools stop thinking that Hadoop is a project dominated by shell scripts. smile
Unlike the days of old, I would not even include the autoconf/automake generated stuff in the source tarball.
Excellent. I filed HADOOP-6436. Will take a stab at it while waiting for reviews on symlink patches =)
Latest patch. Addresses a test failure from test-core and adds some tests.
This includes HADOOP-6427 code. Is that intentional? Should this depend on that issue?
The isQualified test is an error check. Ideally the namenode resolves all links within the file system so all the UnresolvedLinkExceptions that FileContext sees have fully qualified targets. In the current patch the namenode throws an exception to the client even if the link is within the same file system, but even then we want to check that we got back a fully qualified path be clear that the interpretation of the link target is done by the AbstractFileSystem and not FileContext. So I think we need a function to determine if a path is fully qualified in any case right? It also serves to define what we mean by "fully qualified" in Path (this is currently defined implicitly in the makeQualfied method).
I took your (excellent) suggestion of storing the link target verbatim but have the client return a fully qualified path. That's definitely the way to go. The current semantics follow, will update the design doc. All of these cases are now covered in individual tests in TestLink and TestLocalFsLink.
- Relative link targets
fc.createSymlink("file", "/dir/link") creates a link named "link" in /dir (assuming the current working directory is "/dir") that points to "file", eg resolves to "/dir/file", and if "dir" is renamed "dir2" the link resolves to "/dir2/link" because the path is not stored relative. The file context is determined by the parent of the link, eg the fully qualified path of the link target of "/dir/link" is determined by "/dir" not the file context that is used to access the link.
- Absolute links targets
fc.createSymlink("/dir/file", "/dir/link") creates a link named "link" in /dir that points to "/dir/file", if "dir" is renamed "dir2" the link becomes dangling because the link target is stored absolute. The file system is determined by the source not the client, eg fc.open("hdfs://host1/dir/link") opens "/dir/file" on host1 even if accessed using a local file context.
- Fully qualified link targets
fc.createSymlink("hdfs://host/dir/file", "/dir/link") creates a link named "link" in /dir that always points to the fully qualified path specified, regardless of the file context or path used to access the link.
Latest patch implements the semantics and adds tests according to the previous update.
Your latest desciption of relative/absolute/fullyqualified link targets make sense. +1.
Hey Dhruba,
Just to confirm, you mean the description posted in HDFS-245 right?
Which is different from the one in the comment in HADOOP-6421.
Thanks,
Eli
On Fri, Dec 18, 2009 at 6:23 AM, dhruba borthakur (JIRA)
Attached latest patch.
- Implements path resolution as discussed in
HDFS-245andHADOOP-6427 - Added an interface to AbstractFileSystem to indicate if the file system supports symlinks, and a test in FileContextMainOperationsBaseTest to test the behavior of createSymlink when the file system does not support links.
- Additional tests in TestLocalFsLink
- Resolved against trunk, passes test-core
- Removed the changes to HarFileSystem and FsShell so the APIs are only accessible via the tests.
Here's the latest patch that implements the resolution behavior posted to HDFS-245 that Doug, Sanjay and I discussed. Rebased against trunk and passes test-core.
-1 overall. Here are the results of testing the latest attachment
http://issues.apache.org/jira/secure/attachment/12429579/symlink29-common.patch
against trunk revision 896641.
+1 @author. The patch does not contain any @author tags.
+1 tests included. The patch appears to include 50 new or modified tests.
-1 javadoc. The javadoc tool appears to have generated 1 warning messages.
+1 javac. The applied patch does not increase the total number of javac compiler warnings.
-1 findbugs. The patch appears to introduce 3 new Findbugs warnings.
+1 release audit. The applied patch does not increase the total number of release audit warnings.
+1 core tests. The patch passed core unit tests.
+1 contrib tests. The patch passed contrib unit tests.
Test results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/260/testReport/
Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/260/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
Checkstyle results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/260/artifact/trunk/build/test/checkstyle-errors.html
Console output: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/260/console
This message is automatically generated.
Update the last patch to address the find bugs warnings, replaced instantiating a Boolean with Boolean.valueOf in three places in FileContext.
-1 overall. Here are the results of testing the latest attachment
http://issues.apache.org/jira/secure/attachment/12429601/symlink29-common.patch
against trunk revision 896691.
+1 @author. The patch does not contain any @author tags.
+1 tests included. The patch appears to include 50 new or modified tests.
-1 javadoc. The javadoc tool appears to have generated 1 warning messages.
+1 javac. The applied patch does not increase the total number of javac compiler warnings.
+1 findbugs. The patch does not introduce any new Findbugs warnings.
+1 release audit. The applied patch does not increase the total number of release audit warnings.
+1 core tests. The patch passed core unit tests.
+1 contrib tests. The patch passed contrib unit tests.
Test results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/262/testReport/
Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/262/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
Checkstyle results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/262/artifact/trunk/build/test/checkstyle-errors.html
Console output: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/262/console
This message is automatically generated.
-1 overall. Here are the results of testing the latest attachment
http://issues.apache.org/jira/secure/attachment/12429611/symlink29-common.patch
against trunk revision 896691.
+1 @author. The patch does not contain any @author tags.
+1 tests included. The patch appears to include 50 new or modified tests.
-1 javadoc. The javadoc tool appears to have generated 1 warning messages.
+1 javac. The applied patch does not increase the total number of javac compiler warnings.
+1 findbugs. The patch does not introduce any new Findbugs warnings.
+1 release audit. The applied patch does not increase the total number of release audit warnings.
+1 core tests. The patch passed core unit tests.
+1 contrib tests. The patch passed contrib unit tests.
Test results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/263/testReport/
Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/263/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
Checkstyle results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/263/artifact/trunk/build/test/checkstyle-errors.html
Console output: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/263/console
This message is automatically generated.
Not sure what the javadoc warning is about, ant javadoc and ant javadoc-dev are both clean on my host and I've pulled the latest from trunk.
Same patch different name. Hudson used the old copy of the previous patch.
+1 overall. Here are the results of testing the latest attachment
http://issues.apache.org/jira/secure/attachment/12429620/symlink30-common.patch
against trunk revision 896691.
+1 @author. The patch does not contain any @author tags.
+1 tests included. The patch appears to include 50 new or modified tests.
+1 javadoc. The javadoc tool did not generate any warning messages.
+1 javac. The applied patch does not increase the total number of javac compiler warnings.
+1 findbugs. The patch does not introduce any new Findbugs warnings.
+1 release audit. The applied patch does not increase the total number of release audit warnings.
+1 core tests. The patch passed core unit tests.
+1 contrib tests. The patch passed contrib unit tests.
Test results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/265/testReport/
Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/265/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
Checkstyle results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/265/artifact/trunk/build/test/checkstyle-errors.html
Console output: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/265/console
This message is automatically generated.
- DeletegateToFileSystem
shouldn't createSymlink and getLinkTarget throw an exception?
- FilterFs - typo
setTimes() added a blank line.
- TestLocalFsLink
The following two should be in setUp and tearDown respectively; will make writing new tests easier and will also be less error prone (e.g. so that delete is not forgotten).- Path dir = new Path("file:///tmp/test"); // should be in setUp
- fc.delete(dir, true); // should be in tearDown
- TestLocalFsLink
Suggestion: follow the other tests fby setting up a base test and have subclasses that implement the tests for specific file systems.
For examples see: FileContextCreateMkdirBaseTest and TestLocalFSFileContextCreateMkdir).
I noticed that the symlink tests for HDFS are significantly more comprehensive. Would it have been possible to share the tests for both file systems
using the base and subclasses as in other tests?
+1 Looks good except for the above. Will get to your other symlink related jiras in next 2-3 days.
DeletegateToFileSystem. shouldn't createSymlink and getLinkTarget throw an exception?
Good catch, they now both throw an IOException indicating symlinks are not supported. For uniformity, I made getFileLinkStatus do the same, and modified createSymlink to pass the IOException up rather than return a boolean. This way if the user tries to create or access a symlink on a file system that does not support them an IOException is thrown in all cases. I also added tests for getLinkTarget and getFileLinkStatus to testUnsupportedSymlink to verify the behavior for each method when links are unsupported. All relevant javadocs updated.
FilterFs - typo setTimes() added a blank line.
The blank line was already there, the patch removes it.
TestLocalFsLink. The following two should be in setUp and tearDown respectively;
Fixed. Removed empty method clusterTeardown (was @AfterClass) and added cleanupDirs (@After).
TestLocalFsLink. Suggestion: follow the other tests fby setting up a base test and have subclasses that implement the tests for specific file systems.
Done. I created FileContextSymlinkBaseTest and made the test classes for the local file system and HDFS extend it.
I noticed that the symlink tests for HDFS are significantly more comprehensive. Would it have been possible to share the tests for both file systems using the base and subclasses as in other tests?
Yup. With some refactoring I was able to move the bulk of the tests to the common class. It has 33 tests, the local file system class has 4, and the HDFS class now has 13. This uncovered an additional change I needed to make in AbstractFileSystem#rename to avoid throwing a ParentNotDirectoryException if the parent is a symlink.
Thanks for the review! I updated the diffs over on HDFS-245 as well.
+1 overall. Here are the results of testing the latest attachment
http://issues.apache.org/jira/secure/attachment/12429987/symlink31-common.patch
against trunk revision 897023.
+1 @author. The patch does not contain any @author tags.
+1 tests included. The patch appears to include 12 new or modified tests.
+1 javadoc. The javadoc tool did not generate any warning messages.
+1 javac. The applied patch does not increase the total number of javac compiler warnings.
+1 findbugs. The patch does not introduce any new Findbugs warnings.
+1 release audit. The applied patch does not increase the total number of release audit warnings.
+1 core tests. The patch passed core unit tests.
+1 contrib tests. The patch passed contrib unit tests.
Test results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/269/testReport/
Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/269/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
Checkstyle results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/269/artifact/trunk/build/test/checkstyle-errors.html
Console output: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/269/console
This message is automatically generated.
This may refer to an already-committed JIRA, but it seems odd to me that getFileLinkStatus throws when called on a non-link-supporting system. Isn't the idea to replicate lstat semantics? lstat on a non-link is identical to stat, and I'd expect the same of getFileLinkStatus.
Nice catch Todd, was over zealous in my uniformity. Here's the behavior I proposed in HDFS-245:
getLinkFileStatus - like lstat, if the given path refers to a symlink then the FileStatus of the symlink is returned, otherwise the results as if getFileStatus was called. If symlink support is not enabled or the underlying filesystem does not support symlinks then the results are the same as if getFileStatus was called.
The attached patch reverts to the old behavior that getFileLinkStatus is equivalent to getFileStatus if the underlying file system does not support symlinks.
+1 overall. Here are the results of testing the latest attachment
http://issues.apache.org/jira/secure/attachment/12429997/symlink32-common.patch
against trunk revision 897023.
+1 @author. The patch does not contain any @author tags.
+1 tests included. The patch appears to include 15 new or modified tests.
+1 javadoc. The javadoc tool did not generate any warning messages.
+1 javac. The applied patch does not increase the total number of javac compiler warnings.
+1 findbugs. The patch does not introduce any new Findbugs warnings.
+1 release audit. The applied patch does not increase the total number of release audit warnings.
+1 core tests. The patch passed core unit tests.
+1 contrib tests. The patch passed contrib unit tests.
Test results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/270/testReport/
Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/270/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
Checkstyle results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/270/artifact/trunk/build/test/checkstyle-errors.html
Console output: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/270/console
This message is automatically generated.
While updating the design doc I noticed a missing test. This patch adds testLinkStatusAndTargetWithNonLink to show the behavior of getLinkTarget
when called on a normal file (throws an IOException ala how
readlink returns EINVAL in this case). I also made getLinkTarget in
DelegateToFileSystem throw an AssertionError since that method should
never be called--any file system implementation that supports
symlinks and throws an UnresolvedLinkException needs to override
getLinkTarget.
+1 overall. Here are the results of testing the latest attachment
http://issues.apache.org/jira/secure/attachment/12430209/symlink33-common.patch
against trunk revision 898740.
+1 @author. The patch does not contain any @author tags.
+1 tests included. The patch appears to include 15 new or modified tests.
+1 javadoc. The javadoc tool did not generate any warning messages.
+1 javac. The applied patch does not increase the total number of javac compiler warnings.
+1 findbugs. The patch does not introduce any new Findbugs warnings.
+1 release audit. The applied patch does not increase the total number of release audit warnings.
+1 core tests. The patch passed core unit tests.
+1 contrib tests. The patch passed contrib unit tests.
Test results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/272/testReport/
Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/272/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
Checkstyle results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/272/artifact/trunk/build/test/checkstyle-errors.html
Console output: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/272/console
This message is automatically generated.
Oops. I fogot to copy one of my feedback items from my notes.
- createSymlink
- should follow the create() and mkdir() methods of providing a parameter to indicate if missing parent dirs should be created.
- return parameter - somewhat religious - my preference is for the return to be void. Matches mkdir.
Javadoc for createSymlink.
I realize that you are writing a document for the spec and usage model for sym links, however, I think the javadoc for createSymlink should be reasonably complete. I suggest that you include the following:
- kinds of symlinks (to another FS, dot relative, volume-root relative etc.)
- [dangling links are allowed.]
- permissions
- the permissions for the link itself is really that of the directory in which it sits. Hence you can change it if you have permission on the directory
- what permissions apply when you follow links.
- A statement on how the methods deal with a symlink. You have an nice writeup in
HDFS-245(comment dated 15/oct/09). My personal preference is to describe what the other methods do when crossing a symlink in the createSymlink javadoc rather than the the javadoc of the methods themselves.
the permissions for the link itself is really that of the directory in which it sits. Hence you can change it if you have permission on the directory
FWIW, I (still) think not having real ownership/permissions on symlinks is going to be a bad idea in the long run.
[My standard example still holds: Take for example a full HDFS. As an admin, I want to point one or two files/dirs in that dir to somewhere else but leave the rest of the dir writable. This means that any user can change the link and break something that they shouldn't be able to break.]
I think that's reasonable and I'd be willing to implement lchown in a follow on jira. Though I think we should leave the deafault perm behavior as is.
Hey Sanjay,
createSymlink
should follow the create() and mkdir() methods of providing a parameter to indicate if missing parent dirs should be created.
Agreed, will do. I'm not going to add a convenience method that doesn't take the createParent argument.
return parameter - somewhat religious - my preference is for the return to be void. Matches mkdir.
Changed that in the previous patch, it returns void.
Will update the java docs per the above. The latest description of the new APIs follows, will update the design doc.
Thanks,
Eli
FileContext
- FileContext#getLinkFileStatus – If the final path component refers to a symlink then the FileStatus of the symlink is returned. If the the final path component does not refer to a symlink, or the underlying filesystem does not support symlinks, then the result is the same as if getFileStatus was called (like lstat).
- FileContext#getLinkTarget – If the final path component refers to a symlink then the target of the symlink is returned. Links within the path leading up to the final path component are resolved transparently. The target returned is exactly as it was specified in createSymlink. If the the final path component does not refer to a symlink then an IOException is thrown (like readlink which returns EINVAL if the named file is not a symbolic link).
- FileContext#createSymlink – Creates a symbolic link to an existing file. An exception is thrown if the link exits, the user does not have permission to create link, or the underlying file system does not support symlinks.
FileStatus
- FileStatus#isSymlink – Returns true if the FileStatus represents a symbolic link.
- FileStatus#getSymlink – If the FileStatus represents a sylink then the target of the symlink is returned. The symlink target returned is exactly as it was specified in createSymlink. Otherwise an IOException is thrown indicating the FileStatus does not refer to a symlink.
AbstractFileSystem
- AbstractFileSystem#supportsSymlinks – Returns true if the file system supports symlinks, false otherwise.
- AbstractFileSystem#createSymlink – Like FileContext#createSymlink. An IOException is thrown if the file system does not support symlinks.
- AbstractFileSystem#getLinkTarget – Like FileContext#createSymlink. This method is only called by FileContext in response to an UnresolvedLinkException.
- AbstractFileSystem#getFileLinkStatus – Like FileContext#getFileLinkStatus. Except that an UnresolvedLinkException may be thrown if a symlink is encountered in the path leading up to the final path component.
> FWIW, I (still) think not having real ownership/permissions on symlinks is going to be a bad idea in the long run.
I think we can defer this to a follow on JIRA.
Patch attached. FileContext#createSymlink now has a createParent parameter. I left the directory (and link) permission out of the FileContext interface, don't think it's complex enough to justify CreateOpts. I did plumb the link permissions up to ClientProtocol so if we wanted to have FileContext#createSymlink specify link permissions that would be a client side only change. I also added a common symlink test to verify that the parent is created when createParent is true and not when createParent is false. Updated the javadocs per your comment and to reflect the API change; ant javadoc and javadoc-dev are clean. Rebased against the latest bits from trunk.
+1 overall. Here are the results of testing the latest attachment
http://issues.apache.org/jira/secure/attachment/12430484/symlink34-common.patch
against trunk revision 899866.
+1 @author. The patch does not contain any @author tags.
+1 tests included. The patch appears to include 15 new or modified tests.
+1 javadoc. The javadoc tool did not generate any warning messages.
+1 javac. The applied patch does not increase the total number of javac compiler warnings.
+1 findbugs. The patch does not introduce any new Findbugs warnings.
+1 release audit. The applied patch does not increase the total number of release audit warnings.
+1 core tests. The patch passed core unit tests.
+1 contrib tests. The patch passed contrib unit tests.
Test results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/279/testReport/
Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/279/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
Checkstyle results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/279/artifact/trunk/build/test/checkstyle-errors.html
Console output: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/279/console
This message is automatically generated.
Yes, but it needs to be checked in simultaneously with the patch for HDFS-245 (otherwise the build will fail). Sanjay, have you gotten a chance to look at the HDFS-245 patch yet?
+1 overall. Here are the results of testing the latest attachment
http://issues.apache.org/jira/secure/attachment/12431923/symlink35-common.patch
against trunk revision 904975.
+1 @author. The patch does not contain any @author tags.
+1 tests included. The patch appears to include 15 new or modified tests.
+1 javadoc. The javadoc tool did not generate any warning messages.
+1 javac. The applied patch does not increase the total number of javac compiler warnings.
+1 findbugs. The patch does not introduce any new Findbugs warnings.
+1 release audit. The applied patch does not increase the total number of release audit warnings.
+1 core tests. The patch passed core unit tests.
+1 contrib tests. The patch passed contrib unit tests.
Test results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/309/testReport/
Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/309/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
Checkstyle results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/309/artifact/trunk/build/test/checkstyle-errors.html
Console output: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/309/console
This message is automatically generated.
+1 overall. Here are the results of testing the latest attachment
http://issues.apache.org/jira/secure/attachment/12434650/symlink36-common.patch
against trunk revision 905860.
+1 @author. The patch does not contain any @author tags.
+1 tests included. The patch appears to include 14 new or modified tests.
+1 javadoc. The javadoc tool did not generate any warning messages.
+1 javac. The applied patch does not increase the total number of javac compiler warnings.
+1 findbugs. The patch does not introduce any new Findbugs warnings.
+1 release audit. The applied patch does not increase the total number of release audit warnings.
+1 core tests. The patch passed core unit tests.
+1 contrib tests. The patch passed contrib unit tests.
Test results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/323/testReport/
Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/323/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
Checkstyle results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/323/artifact/trunk/build/test/checkstyle-errors.html
Console output: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/323/console
This message is automatically generated.
Feedback on patch patch 33, FileContextSymlinkBaseTest
- setup should create the test dir.
- testCreateFileViaSymlink()
it does not create the file via the symlink - file is created directly; it is only read via symlink.
- testCreateFileExistingLink
rename to testCreateFileDirExitingLink
add a mkdir that fails.
- testCreateLinkUsingRelPath
rename testCreateLinkToFileUsingRelPath to match with testCreateLinkToFileUsingXXX
- testCreateLinkToFileUsingXXX
in testCreateLinkToFileUsingAbsPath you read the file "using rel, abs, and qualified paths"
It would make sense to do that for the other two tests.
- testSetWDResolvesLink
Uses a link with an abs path.
For this test it would worth adding test for all 3 symlinks (rel, abs, qual) since the
code-path here is different than testCreateLinkUsingXXX and will not be covered by those tests.
- Structure and coverage:
Noticed that testCreateLinkXX has tests for abs, relative and FQ links.
Most of the other tests use an abs link.
You also have separate tests that test relative, abs, FQ links but they only test specific methods but not all.
So I am somewhat unclear about the structure of the tests in how they cover all 3 kinds of links.
- Use the test library - FileContextTestHelper.java - it will make your tests simpler.
Patch attached adds default implementations for AbstractFileSystem so new file system implementations don't need to override the various symlink APIs, just as existing FileSystem based ones don't because DelegateToFileSystem provides default implementations. This allows the common patch to be checked in independently of the HDFS one. I'm running test-core on a latest trunk HDFS repo against a latest common repo with this patch applied to make sure it can in fact go in independently. So far so good.
Forgot to mention, patches 34 through 36 addressed Sanjay's feedback in the previous comment.
+1 overall. Here are the results of testing the latest attachment
http://issues.apache.org/jira/secure/attachment/12435010/symlink37-common.patch
against trunk revision 906388.
+1 @author. The patch does not contain any @author tags.
+1 tests included. The patch appears to include 14 new or modified tests.
+1 javadoc. The javadoc tool did not generate any warning messages.
+1 javac. The applied patch does not increase the total number of javac compiler warnings.
+1 findbugs. The patch does not introduce any new Findbugs warnings.
+1 release audit. The applied patch does not increase the total number of release audit warnings.
+1 core tests. The patch passed core unit tests.
+1 contrib tests. The patch passed contrib unit tests.
Test results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/333/testReport/
Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/333/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
Checkstyle results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/333/artifact/trunk/build/test/checkstyle-errors.html
Console output: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/333/console
This message is automatically generated.
My final review comments on patch 37.
Thanks for addressing my previous comments; Thanks for declaring the exception; however you missed/have mistakes on a few.
- Javadoc : FileContext#createSymLink
- you added more details in Javadoc but not the semantics of the operations
- suggestion add:
- The following operations follow (ie resolve through) the symlinks: open, set/getWorkingDir, setReplication, setPermission,
setOwner, setTimes, get/setFileChecksum, getFileStatus, isDirectory, isFile, listStatus, getFileBlockLocations, getFsStatus. - The following operations follow intermediate symlinks in the path but not the final leaf symlink:
create/mkdir the leaf should not exit.
delete, deleteOnExit removes the leaf symlink
- The following operations follow (ie resolve through) the symlinks: open, set/getWorkingDir, setReplication, setPermission,
- UnresolvedLinkException
- AbstractFileSystem#getServerDefaults should not throw the UnresolvedLinkException
- AbstractFileSystem#getLinkTarget - should this throw UnresolvedLinkException?
- Why does Filterfs throw UnresolvedLinkException on only some of its methods with a path name arg and not all.
- RawLocalFs and symlinks – I missed this in my earlier comments.
RawLocalFs is suppose to support only volume root relative and dot relative symlinks (links to other file systems
are not supported). Noticed that you strip out the scheme - instead throw an exception if the link is to a different file system.
(You will also have to fix the test that takes advantage of this).
Some comments on tests in my next comment.
Could you also add some javadoc for T and the public methods in FSLinkResolver?
Comments on the tests for patch 37.
Thanks for addressing my comments.
- testLocalFSFileContextSymlink#testGetLinkStatusPartQualTarget - you create a link "hdfs://host:1000/dir/file". This link should fail on local fs. (see
my previous comment).
- Move the setPermissions and setTimes tests from HDFS to Common (you can do this in a separate jira if you prefer, though you might find it
easier to do in this patch.)
- File a jira to use the test library (your issue with the test library and eclipse noted). We need to fix the test library to address the eclipse issue and
then change the symlink test. The other fileContext tests use the test library.
The attached patch incorporates the feedback from you and Nicholas.
- AFS#getLinkTarget will never thrown an UnresolvedLinkException because it resolves the first symlink in the path. See NameNode#getLinkTarget for details.
- Left the permissions test as is and filed HDFS-971 to add symlinks to TestDFSPermission once it's converted to FileContext, and HADOOP-6561 to add symlink coverage to TestFcLocalFsPermission.
- Moved the setTimes test to the base test class though the asserts are only checked for non-local file systems as FileSystem#setTimes is a nop and none of the local file system classes override it.
- Filed
HADOOP-6562to make FileContextSymlinkBaseTest use FileContextTestHelper onceHADOOP-5916is resolved.
Patch also merges with trunk. ant test-core passes.
Kicking Hudson, didn't seem to pick up on the submit patch from yesterday.
Here are ant test-patch results in case Hudson continues to have problems.
[exec] ======================================================================
[exec] ======================================================================
[exec]
[exec]
[exec] /home/eli/apache-ant-1.7.1/bin/ant -Dversion=PATCH-symlink39-common.patch -Djava5.home=/usr/lib/jvm/jdk1.5.0_21 -Dforrest.home=/home/eli/apache-forrest-0.8 -DHadoopPatchProcess= releaseaudit > /home/eli/tmp/patchReleaseAuditWarnings.txt 2>&1
[exec]
[exec]
[exec] There appear to be 1 release audit warnings before the patch and 1 release audit warnings after applying the patch.
[exec]
[exec]
[exec]
[exec]
[exec] +1 overall.
[exec]
[exec] +1 @author. The patch does not contain any @author tags.
[exec]
[exec] +1 tests included. The patch appears to include 14 new or modified tests.
[exec]
[exec] +1 javadoc. The javadoc tool did not generate any warning messages.
[exec]
[exec] +1 javac. The applied patch does not increase the total number of javac compiler warnings.
[exec]
[exec] +1 findbugs. The patch does not introduce any new Findbugs warnings.
[exec]
[exec] +1 release audit. The applied patch does not increase the total number of release audit warnings.
[exec]
[exec]
[exec]
[exec]
[exec] ======================================================================
[exec] ======================================================================
+1 overall. Here are the results of testing the latest attachment
http://issues.apache.org/jira/secure/attachment/12435793/symlink39-common.patch
against trunk revision 909806.
+1 @author. The patch does not contain any @author tags.
+1 tests included. The patch appears to include 14 new or modified tests.
+1 javadoc. The javadoc tool did not generate any warning messages.
+1 javac. The applied patch does not increase the total number of javac compiler warnings.
+1 findbugs. The patch does not introduce any new Findbugs warnings.
+1 release audit. The applied patch does not increase the total number of release audit warnings.
+1 core tests. The patch passed core unit tests.
+1 contrib tests. The patch passed contrib unit tests.
Test results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h1.grid.sp2.yahoo.net/2/testReport/
Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h1.grid.sp2.yahoo.net/2/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
Checkstyle results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h1.grid.sp2.yahoo.net/2/artifact/trunk/build/test/checkstyle-errors.html
Console output: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h1.grid.sp2.yahoo.net/2/console
This message is automatically generated.
Integrated in Hadoop-Common-trunk-Commit #169 (See http://hudson.zones.apache.org/hudson/job/Hadoop-Common-trunk-Commit/169/)
Adds Symbolic links to FileContext, AbstractFileSystem.
It also adds a limited implementation for the local file system
(RawLocalFs) that allows local symlinks. (Eli Collins via Sanjay Radia)
Integrated in Hadoop-Common-trunk #252 (See http://hudson.zones.apache.org/hudson/job/Hadoop-Common-trunk/252/)
Adds Symbolic links to FileContext, AbstractFileSystem.
It also adds a limited implementation for the local file system
(RawLocalFs) that allows local symlinks. (Eli Collins via Sanjay Radia)
I missed that this breaks compatibility in a subtle way:
Negation of FileStatus#isDir() is normally used to see if the target is a file. This will now break if the target happens to be symlink.
I suggest we deprecate isDir(), remove isSymlink() and add new method that returns an enum.
Alternatively we could deprecate isDir() and add isDirectory() and isFile();
Nice catch Sanjay.
Alternatively we could deprecate isDir() and add isDirectory() and isFile();
This sounds good to me. I'll send out a patch.
+1 for adding isDirectory() and isFile()
+1 here too.. makes it easier to add new types of INodes that are neither files nor directories
Hey Zlatin,
The intent for FileStatus (and FileContext) is to expose a least common denominator API for all file systems that Hadoop uses (HDFS, Local, S3, etc) so file system specific stuff like dev, proc, etc are all things we'd like to keep out of the API.
Thanks,
Eli
Filed HADOOP-6585 for the new API and changing common uses of isDir(), filed HDFS-995 for replacing HDFS uses of isDir().
Integrated in Hadoop-Hdfs-trunk-Commit #201 (See http://hudson.zones.apache.org/hudson/job/Hadoop-Hdfs-trunk-Commit/201/)
HDFS-245. Adds a symlink implementation to HDFS. This complements the new symlink feature added in
(Eli Collins via Sanjay Radia)
Integrated in Hdfs-Patch-h2.grid.sp2.yahoo.net #146 (See http://hudson.zones.apache.org/hudson/job/Hdfs-Patch-h2.grid.sp2.yahoo.net/146/)
Integrated in Hdfs-Patch-h5.grid.sp2.yahoo.net #302 (See http://hudson.zones.apache.org/hudson/job/Hdfs-Patch-h5.grid.sp2.yahoo.net/302/)
Integrated in Hadoop-Hdfs-trunk #275 (See http://hudson.zones.apache.org/hudson/job/Hadoop-Hdfs-trunk/275/)
Latest patch attached.