-
Notifications
You must be signed in to change notification settings - Fork 8.9k
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
HDFS-16791 WIP - client protocol and Filesystem apis implemented and … #4967
HDFS-16791 WIP - client protocol and Filesystem apis implemented and … #4967
Conversation
💔 -1 overall
This message was automatically generated. |
hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/FileSystem.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
Show resolved
Hide resolved
...s-project/hadoop-hdfs-client/src/main/java/org/apache/hadoop/hdfs/DistributedFileSystem.java
Outdated
Show resolved
Hide resolved
...project/hadoop-hdfs-client/src/main/java/org/apache/hadoop/hdfs/protocol/ClientProtocol.java
Outdated
Show resolved
Hide resolved
...-rbf/src/test/java/org/apache/hadoop/hdfs/server/federation/router/TestRouterMountTable.java
Show resolved
Hide resolved
...-rbf/src/main/java/org/apache/hadoop/hdfs/server/federation/router/RouterClientProtocol.java
Outdated
Show resolved
Hide resolved
...fs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/fs/viewfs/TestViewFileSystemHdfs.java
Outdated
Show resolved
Hide resolved
...fs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/fs/viewfs/TestViewFileSystemHdfs.java
Outdated
Show resolved
Hide resolved
hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/TestEnclosingRoot.java
Outdated
Show resolved
Hide resolved
I was expecting |
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.
- update filesystem.md
- needs contract tests to verify base behaviour and subclasses
test to pass in /, home dir, Path from a different fs.
hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/FileSystem.java
Outdated
Show resolved
Hide resolved
hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/FileSystem.java
Show resolved
Hide resolved
hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/FileSystem.java
Show resolved
Hide resolved
...p-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/viewfs/ViewFileSystem.java
Show resolved
Hide resolved
...s-project/hadoop-hdfs-client/src/main/java/org/apache/hadoop/hdfs/DistributedFileSystem.java
Outdated
Show resolved
Hide resolved
...project/hadoop-hdfs-client/src/main/java/org/apache/hadoop/hdfs/protocol/ClientProtocol.java
Outdated
Show resolved
Hide resolved
hadoop-hdfs-project/hadoop-hdfs-client/src/main/proto/ClientNamenodeProtocol.proto
Outdated
Show resolved
Hide resolved
...-rbf/src/main/java/org/apache/hadoop/hdfs/server/federation/router/RouterClientProtocol.java
Show resolved
Hide resolved
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.
lgtm
💔 -1 overall
This message was automatically generated. |
💔 -1 overall
This message was automatically generated. |
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.
Looking at the specification, I'm not sure that getRootPath()
is the right name, given that the root dir is "/" everywhere -and very special (you can delete it and it is still there afterwards). (it defined in model.md)
What about getMountRoot()
, as that is clearer at what it is.
Also, how about in the spec we provide details/hints about what it means if two parts share the same mount root -or are different.
For example:
For any two paths p1 and p2 in a
mount roots are not equal, rename(p1, p2) is likely to either fail or be emulated by (a potentially slow) copy operation, and so may not be atomic.
Is the mount root always a directory? again, say so.
return a path r
where:
r in directories(FS)
r = p or r in ancestors(p)
what is getRootPath(getRootPath(p))
? is it always the same as getRootPath(p)
? i believe it should be, in which case it must be included in the specification, then tested
for all tests of the specification which are expected to be met by all filesystems -not just hdfs/viewfs- we need a contract test subclass of AbstractFSC
I think you've already written a lot of that test, it's just needs to be moved into hadoop-common, given implementations through localfs (which is an imp
Tests I can imagine
root path resolves to being equal to or ancestor of path passed in
same path resolves to same mount root if invoked twice
wrapped invocation is unchanged.
works for a path which exists
works for a path which doesn't exist
returned path exists and is a directory
(yes, this is a lot of extra work -but it is how we get stronger definitions of foundational behaviours than "look at what hdfs does and try to copy it")
hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/FileSystem.java
Outdated
Show resolved
Hide resolved
...s-project/hadoop-hdfs-client/src/main/java/org/apache/hadoop/hdfs/DistributedFileSystem.java
Outdated
Show resolved
Hide resolved
...-rbf/src/test/java/org/apache/hadoop/hdfs/server/federation/router/TestRouterMountTable.java
Outdated
Show resolved
Hide resolved
...ain/java/org/apache/hadoop/hdfs/protocolPB/ClientNamenodeProtocolServerSideTranslatorPB.java
Outdated
Show resolved
Hide resolved
...ain/java/org/apache/hadoop/hdfs/protocolPB/ClientNamenodeProtocolServerSideTranslatorPB.java
Outdated
Show resolved
Hide resolved
hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/TestEnclosingRoot.java
Show resolved
Hide resolved
cluster.shutdown(); | ||
cluster = null; | ||
} | ||
EncryptionFaultInjector.instance = new EncryptionFaultInjector(); |
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.
put in a finally clause in case cluster.shutdown fails?
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 can add it, but we don't do this anywhere else in tests that do shutdown or in the encryption tests
hadoop-common-project/hadoop-common/src/site/markdown/filesystem/filesystem.md
Show resolved
Hide resolved
hadoop-common-project/hadoop-common/src/site/markdown/filesystem/filesystem.md
Outdated
Show resolved
Hide resolved
hadoop-common-project/hadoop-common/src/site/markdown/filesystem/filesystem.md
Outdated
Show resolved
Hide resolved
@steveloughran I can't comment on your long comment above,
The current name being proposed is As for the tests you listed, I have all of them somewhere except for
For the below, im not sure what you are referring to. I see
|
e9c0bd2
to
5d2a96c
Compare
look at all the subclasses of |
@steveloughran I think I found it. I just updated the PR with the contract tests. It looks pretty comparable to other contract test suites. lmk if there is still something missing. |
💔 -1 overall
This message was automatically generated. |
💔 -1 overall
This message was automatically generated. |
💔 -1 overall
This message was automatically generated. |
@steveloughran as I commented before, I added all the contract tests I could find that made sense. Some level of functionality couldn't practically be tested in the contract tests because they require some idea of mount points or encyrption zones. As far as the build failures go...
Let me know if there is anything else missing. |
💔 -1 overall
This message was automatically generated. |
hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/TestEnclosingRoot.java
Show resolved
Hide resolved
hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/viewfs/ViewFs.java
Show resolved
Hide resolved
hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/viewfs/ViewFs.java
Show resolved
Hide resolved
...oop-common/src/test/java/org/apache/hadoop/fs/contract/AbstractContractGetEnclosingRoot.java
Show resolved
Hide resolved
...oop-common/src/test/java/org/apache/hadoop/fs/contract/AbstractContractGetEnclosingRoot.java
Outdated
Show resolved
Hide resolved
hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/FileSystem.java
Outdated
Show resolved
Hide resolved
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.
@mccormickt12 could you remove the whitespace changes in the PR. I think these are being added by the IDE automatically.
…building. Tests for Distributed Filesystem, view filesystem and RBF
3e233fc
to
b6382b0
Compare
💔 -1 overall
This message was automatically generated. |
💔 -1 overall
This message was automatically generated. |
Could you fix the new checkstyle issues: https://ci-hadoop.apache.org/job/hadoop-multibranch/job/PR-4967/11/artifact/out/results-checkstyle-root.txt |
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.
looks good; no fundamental issues.
- I'd like the ViewFS &c catching and wrapping FNFEs to include as the inner cause, so if something really odd happens underneath, we have more info.
- most of the junit assertEquals() asserts are of the form (outcome, expected). for the generated exception to be good, it needs to be other way round.
- AssertJ is even better, if a bit more verbose. I've shown how it needs to be used in the test case using assertThat
hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/AbstractFileSystem.java
Outdated
Show resolved
Hide resolved
hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/FileSystem.java
Show resolved
Hide resolved
hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/FileSystem.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
...oop-common/src/test/java/org/apache/hadoop/fs/contract/AbstractContractGetEnclosingRoot.java
Outdated
Show resolved
Hide resolved
...oop-common/src/test/java/org/apache/hadoop/fs/contract/AbstractContractGetEnclosingRoot.java
Outdated
Show resolved
Hide resolved
...oop-common/src/test/java/org/apache/hadoop/fs/contract/AbstractContractGetEnclosingRoot.java
Outdated
Show resolved
Hide resolved
...s-project/hadoop-hdfs-client/src/main/java/org/apache/hadoop/hdfs/DistributedFileSystem.java
Outdated
Show resolved
Hide resolved
hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/TestEnclosingRoot.java
Outdated
Show resolved
Hide resolved
@steveloughran It seems like you are no longer reviewing, but just pinging in case you are. |
Also just an FYI I am planning to open a new PR. I tried rebasing, but it took a very long time given the number of commits that have stacked up. I am going to just apply the diff on top of branch-3.3 and open a new PR, I will link this one to keep all the context. Happy to address additional PR comments here before doing that though |
💔 -1 overall
This message was automatically generated. |
💔 -1 overall
This message was automatically generated. |
Closing because #6198 (review) got merged |
…building.
Tests for Distributed Filesystem, view filesystem and RBF
Description of PR
How was this patch tested?
hadoop-hdfs % mvn test -Dtest="TestViewFileSystemHdfs"
hadoop-hdfs-rbf % mvn test -Dtest="TestRouterMountTable"
hadoop-hdfs % mvn test -Dtest="TestEnclosingRoot"
hadoop-hdfs % mvn test -Dtest="TestGetEnclosingRoot"
hadoop-common-project % mvn test -Dtest="TestHarFileSystem"
hadoop-hdfs-project % mvn test -Dtest="TestReadOnly"
hadoop-hdfs-project % mvn test -Dtest="TestDistributedFileSystem"
For code changes:
LICENSE
,LICENSE-binary
,NOTICE-binary
files?