HADOOP-15891. provide Regex Based Mount Point In Inode Tree #2185
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
|
💔 -1 overall
This message was automatically generated. |
Member
|
This seems an interesting feature. Do we need to update the |
Contributor
Author
|
@liuml07 Thanks, this is a feature adopted inside our company for almost two years. The code is almost as same as our internal branch except I removed some refactored code to make it easier to review. Seemed like the rebase caused some UT failures. Let me fix the UTs and update the user guide. |
JohnZZGithub
force-pushed
the
HADOOP-15891
branch
from
db45b76 to
65c151a
Compare
Contributor
Author
|
@templedf It will be great if you could help with the review, thanks. |
JohnZZGithub
force-pushed
the
HADOOP-15891
branch
from
65c151a to
1fe5665
Compare
|
💔 -1 overall
This message was automatically generated. |
JohnZZGithub
force-pushed
the
HADOOP-15891
branch
from
1fe5665 to
3ae248e
Compare
|
💔 -1 overall
This message was automatically generated. |
JohnZZGithub
force-pushed
the
HADOOP-15891
branch
from
3ae248e to
44f3909
Compare
|
💔 -1 overall
This message was automatically generated. |
Contributor
Author
|
Here're the UT failed. I didn't see them related. But I could double-check. Thanks [ERROR] Failures: |
JohnZZGithub
force-pushed
the
HADOOP-15891
branch
from
44f3909 to
d3e5b83
Compare
|
💔 -1 overall
This message was automatically generated. |
JohnZZGithub
commented
Aug 7, 2020
Contributor
Author
There was a problem hiding this comment.
It looks complex. But the major change here is simple.
Before:
if (key.startsWith(mountTablePrefix)) {
// hundred line of code
}
After:
if (!key.startsWith(mountTablePrefix)) {
continue
}
// hundred line of code
Contributor
Author
|
The UTs failure are not related to the change. Here're some references: |
JohnZZGithub
force-pushed
the
HADOOP-15891
branch
from
d3e5b83 to
c58b945
Compare
Contributor
Author
|
Just did a rebase and updated the PR. |
|
💔 -1 overall
This message was automatically generated. |
Contributor
|
I will review it in a day or two, Thanks |
Contributor
Author
|
Thank you! let me check ViewFs.java. Function wise, this patch worked for MR jobs and HDFS use cases in our internal clusters.
|
Contributor
umamaheswararao
left a comment
umamaheswararao
left a comment
There was a problem hiding this comment.
@JohnZZGithub , Thanks a lot for the work.
I have quickly gone throw the patch and posted few comments. I will continue review if I can find more comments. It's a nice feature! Great work!
hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/viewfs/InodeTree.java
Outdated
Show resolved
Hide resolved
hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/viewfs/ConfigUtil.java
Outdated
Show resolved
Hide resolved
hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/viewfs/InodeTree.java
Outdated
Show resolved
Hide resolved
hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/viewfs/InodeTree.java
Outdated
Show resolved
Hide resolved
hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/viewfs/InodeTree.java
Outdated
Show resolved
Hide resolved
...-project/hadoop-common/src/test/java/org/apache/hadoop/fs/viewfs/ViewFileSystemBaseTest.java
Outdated
Show resolved
Hide resolved
...t/java/org/apache/hadoop/fs/viewfs/TestRegexMountPointResolvedDstPathReplaceInterceptor.java
Outdated
Show resolved
Hide resolved
Contributor
There was a problem hiding this comment.
Not about this test, In general on inner cache disabling: There was a suggestion for avoiding explicitly user disabling it. If that not possible, you may want to have check while analyzing mount points itself that, if cache is enabled and using regex mounts, then you may want to fail fs initialization itself?
Contributor
Author
There was a problem hiding this comment.
@umamaheswararao This is a good point. Let me add some preconditions check.
BTW, now the inner cache assumes every filesystem is created while InodeTree is constructed and never changed. Do you think it's reasonable to change it to a concurrent hash map?
Contributor
Author
There was a problem hiding this comment.
One more thing to clarify, I guess this config is for per schema level cache. Regex mount point is OK with it. Mount table is not good inner cache inside ViewFileSystem.java.
Contributor
There was a problem hiding this comment.
I agree with this issue and filed a JIRA for it. https://issues.apache.org/jira/browse/HADOOP-17028
This issue should be solved once implement that.
Contributor
Author
There was a problem hiding this comment.
I didn't realize that there is a jira. This is great.
...oject/hadoop-hdfs/src/test/java/org/apache/hadoop/fs/viewfs/TestViewFileSystemLinkRegex.java
Outdated
Show resolved
Hide resolved
JohnZZGithub
force-pushed
the
HADOOP-15891
branch
2 times, most recently
from
809b9a1 to
31a68cd
Compare
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
JohnZZGithub
force-pushed
the
HADOOP-15891
branch
from
31a68cd to
f9c61ef
Compare
JohnZZGithub
force-pushed
the
HADOOP-15891
branch
from
375a438 to
8786ee8
Compare
|
💔 -1 overall
This message was automatically generated. |
Contributor
umamaheswararao
left a comment
umamaheswararao
left a comment
There was a problem hiding this comment.
Thanks you @JohnZZGithub for the update. Patch looks awesome.
I have few minors to address before going.
Otherwise I am +1
| Path homeDir = null; | ||
| private boolean enableInnerCache = false; | ||
| private InnerCache cache; | ||
| private boolean evictCacheOnClose = false; |
Contributor
There was a problem hiding this comment.
Do you see any issue if we make it true? If no issues, we can simply clean it on close right instead of having another config?
Seems like this is an improvement to existing code. If you want, I am ok to file small JIRA and fix this cleanup thing.( I am assuming it's not necessarily needed with this.)
Contributor
Author
There was a problem hiding this comment.
@umamaheswararao Thanks for the kindness. I didn't see a problem with making it true. Just mean to be more cautious, let me remove it.
| Assert.assertTrue( | ||
| resolveResult.targetFileSystem | ||
| instanceof TestRegexMountPointFileSystem); | ||
| Assert.assertTrue(resolveResult.resolvedPath.equals("/user/hadoop_user1")); |
Contributor
There was a problem hiding this comment.
Still this asserts can use assertEquals method? Please use them all places.
Contributor
Author
There was a problem hiding this comment.
Nice catch, let me clean them up.
| // Asserts | ||
| URI viewFsUri = new URI( | ||
| FsConstants.VIEWFS_SCHEME, CLUSTER_NAME, "/", null, null); | ||
| vfs = FileSystem.get(viewFsUri, config); |
Contributor
There was a problem hiding this comment.
small suggestion here:
Move above lines out of try block. Then use try(FileSystem vfs = = FileSystem.get(viewFsUri, config)){
}
This should close automatically after block. So, we can remove finally block below?
Contributor
There was a problem hiding this comment.
Is the below comment is still valid?
| ### Understand the Difference | ||
|
|
||
| In the key-value based mount table, view file system treats every mount point as a partition. There's several file system APIs which will lead to operation on all partitions. E.g. there's an HDFS cluster with multiple mount. Users want to run “hadoop fs -put file viewfs://hdfs.namenode.apache.org/tmp/” cmd to copy data from local disk to our HDFS cluster. The cmd will trigger ViewFileSystem to call setVerifyChecksum() method which will initialize the file system for every mount point. | ||
| For a regex-base rule mount table entry, we couldn't know what's corresponding path until parsing. So the regex based mount table entry will be ignored on such cases. The file system (ChRootedFileSystem) will be created upon accessing. But the underlying file system will be cached by inner cache of ViewFileSystem. |
Contributor
There was a problem hiding this comment.
For a regex-base rule mount table entry, we couldn't know what's corresponding path until parsing.
Whatever we know should be added to mountPoints? So, that getMountPoints will return known fs-es? It may be a good idea to add Java doc on API level.
I am ok to have this in followup JIRA to cover all of this aspects.
Contributor
Author
There was a problem hiding this comment.
Good idea. I guess this patch didn't add parsed fs to mount point yet. Maybe it's better when we modify the code and doc at the same time. Created https://issues.apache.org/jira/browse/HADOOP-17247 to track the issue. Does it make sense? Thanks
Contributor
There was a problem hiding this comment.
Thanks for having separate JIRA. It make sense to me. !
|
💔 -1 overall
This message was automatically generated. |
JohnZZGithub
force-pushed
the
HADOOP-15891
branch
from
6c9f1d1 to
4c543ea
Compare
|
💔 -1 overall
This message was automatically generated. |
Contributor
|
Great work @JohnZZGithub. +1 pending jenkins clean report. |
|
💔 -1 overall
This message was automatically generated. |
Contributor
Author
|
@umamaheswararao Thanks a lot!
As for the failure UTs, I guess they are not related to the patch. |
| * @param mountTableName - the mountable name of the regex config item | ||
| * @param srcRegex - the src path regex expression that applies to this config | ||
| * @param targetStr - the string of target path | ||
| */ |
Contributor
There was a problem hiding this comment.
This method not used anywhere?
Contributor
Author
There was a problem hiding this comment.
Good catch, I guess the next addLinkRegex is used but not this one.
Contributor
|
I think the following new lines added into this method. Thats the reason it is reporting longer method warning.. Probably we can extract this logics to separate methods. move the following code to a small check method. There are two places we are doing same checks. At end of the method we have some logics for handle no mount points code. IF we need we could extract that to handleNoMountPoints method or so. Check if this refactoring can make checkstyle happy and code can look cleaner. |
Contributor
Author
|
@umamaheswararao Sure. In fact, I did a major refactor in our internal repo to make it switch case-based. (https://github.com/apache/hadoop/pull/424/files#diff-69fd14ba63365b6a428bf7142c463990R511) However, I didn't put it here as it's harder to review. |
Contributor
|
Thanks @JohnZZGithub I agree it's always better to have refactoring patch separated to make the reviews easier. |
Contributor
Author
|
@umamaheswararao Totally make sense, updated. |
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
Contributor
|
+1 on the latest patch. Thanks again for your great work @JohnZZGithub Test failures are unrelated. |
Contributor
umamaheswararao
left a comment
umamaheswararao
left a comment
There was a problem hiding this comment.
Committed to trunk!
Contributor
Author
|
@umamaheswararao Thanks a lot! |
umamaheswararao
pushed a commit
that referenced
this pull request
Sep 13, 2020
…Contributed by Zhenzhao Wang. Co-authored-by: Zhenzhao Wang <[email protected]> (cherry picked from commit 12a316c)
jojochuang
pushed a commit
to jojochuang/hadoop
that referenced
this pull request
May 23, 2023
). Contributed by Zhenzhao Wang. Co-authored-by: Zhenzhao Wang <[email protected]> (cherry picked from commit 12a316c) Change-Id: Ibabb9d80bfcc634b529866b31f405573eb7f03f2 (cherry picked from commit 4102865)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
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.
https://issues.apache.org/jira/browse/HADOOP-15891