HDFS-15052. WebHDFS getTrashRoot leads to OOM due to FileSystem objec… #1758
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. |
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
The findbugs warning was addressed by HDFS-15048. |
jojochuang
reviewed
Jan 9, 2020
| trashRoot = new org.apache.hadoop.fs.Path( | ||
| new org.apache.hadoop.fs.Path(USER_HOME_PREFIX, user), TRASH_PREFIX); | ||
| } | ||
| return trashRoot.toUri().getPath(); |
There was a problem hiding this comment.
This change assumes namenode's default fs is a DistributedFileSystem, which makes sense.
The change basically copies the implementation inside DistributedFileSystem#getTrashRoot(). If that method evolves in the future, the same changes should be applied in here too.
Can we add a comment inside DistributedFileSystem#getTrashRoot(), that any change should be added in NamenodeWebHdfsMethods#getTrashRoot() too?
There was a problem hiding this comment.
Thanks for the comment. I moved the common logic into DFSUtilClient in the last commit. While added TestWebHDFS#testGetEZTrashRoot could detect the future mismatch, it is not in the same subproject of DistributedFileSystem (hadoop-hdfs-client).
There was a problem hiding this comment.
Discussed with @daryn-sharp on this. New features in the future might require additional logic in determining trash root. It is better to be done at the server-side. So, the Ideal solution would be to add a getTrashRoot() namenode RPC method. DistributedFileSystem would then have a fallback logic for compatibility with older servers. NamenodeWebHdfsMethods would simply call this rpc method.
Also, use of Path on NN is highly discouraged. Path is super-expensive and performs normalization that no other operation performs.
There was a problem hiding this comment.
From the the current implementation, we need to fix the memory leak and the fact it does not work with security enabled, and probably backport that across the active branches. The memory leak especially is a big problem.
Would it make sense to adopt this patch as it stands and push it to all the branches and then open another Jira to implement the namenode RPC suggestion on trunk as a new feature?
There was a problem hiding this comment.
I am fine with an interim solution to fix the immediate issues. But use of Path in NN is far from ideal. It will be best if this is addressed now than later.
There was a problem hiding this comment.
@kihwal Thanks for the comment. I updated the code based on your suggestion.
|
💔 -1 overall
This message was automatically generated. |
|
Failed unit tests are not related to trash logic and passed on my local. |
kihwal
approved these changes
Feb 20, 2020
|
💔 -1 overall
This message was automatically generated. |
|
The unit test failure is unrelated and caused by "BindException: Address already in use" |
|
Thanks. I merged this. trying to backport to relevant branches. |
asfgit
pushed a commit
that referenced
this pull request
Feb 21, 2020
…t creation. (#1758) (cherry picked from commit 2338d25) Conflicts: hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/web/TestWebHDFS.java (cherry picked from commit 610805e) Conflicts: hadoop-hdfs-project/hadoop-hdfs-client/src/main/java/org/apache/hadoop/hdfs/DFSUtilClient.java hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/web/resources/NamenodeWebHdfsMethods.java hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/web/TestWebHDFS.java
bilaharith
pushed a commit
to bilaharith/hadoop
that referenced
this pull request
Mar 19, 2020
RogPodge
pushed a commit
to RogPodge/hadoop
that referenced
this pull request
Mar 25, 2020
bentito
pushed a commit
to bentito/hadoop
that referenced
this pull request
Dec 2, 2020
…t creation. (apache#1758) (cherry picked from commit 2338d25) Conflicts: hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/web/TestWebHDFS.java (cherry picked from commit 610805e)
bentito
pushed a commit
to bentito/hadoop
that referenced
this pull request
Dec 3, 2020
…t creation. (apache#1758) (cherry picked from commit 2338d25) Conflicts: hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/web/TestWebHDFS.java (cherry picked from commit 610805e)
jojochuang
pushed a commit
to jojochuang/hadoop
that referenced
this pull request
May 23, 2023
…t creation. (apache#1758) (cherry picked from commit 2338d25) Conflicts: hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/web/TestWebHDFS.java (cherry picked from commit 610805e) Change-Id: I1ae0e46a18f4d367ee409db63ee98f0c9baf7720
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.
No description provided.