-
Notifications
You must be signed in to change notification settings - Fork 9.2k
HDFS-15982. Deleted data using HTTP API should be saved to the trash #2927
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
Conversation
This comment has been minimized.
This comment has been minimized.
|
@ayushtkn @steveloughran Would you like to take a look? |
|
LGTM (Verified on my local cluster) |
|
Thanks for the review @bhavikpatel9977. Could you please take a look @tasanuma @aajisaka ? |
|
@liuml07 if you have some free cycles to take a look? |
|
@jojochuang if you have some cycles to review this PR? |
66d8c18 to
b706740
Compare
|
Thanks for updating. I like the UI support; it's very handy. If we want to close https://issues.apache.org/jira/browse/HDFS-14320 and move on with this patch, I think we need to update the WebHDFS doc as well (HttpFs is using the same API so no separate doc I assume). |
This comment has been minimized.
This comment has been minimized.
Oh, you mean we should update |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
baf8702 to
72a7c25
Compare
|
@liuml07 done with changes if you would like to review further. |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
@smengcl @steveloughran would you like to take a look? |
smengcl
left a comment
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.
Thanks @virajjasani . Good job adding the change in both WebHDFS and HttpFS there!
...-project/hadoop-hdfs-httpfs/src/main/java/org/apache/hadoop/fs/http/server/FSOperations.java
Show resolved
Hide resolved
...oop-hdfs-client/src/main/java/org/apache/hadoop/hdfs/web/resources/DeleteSkipTrashParam.java
Show resolved
Hide resolved
virajjasani
left a comment
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.
Thanks for the review @smengcl
...oop-hdfs-client/src/main/java/org/apache/hadoop/hdfs/web/resources/DeleteSkipTrashParam.java
Show resolved
Hide resolved
...-project/hadoop-hdfs-httpfs/src/main/java/org/apache/hadoop/fs/http/server/FSOperations.java
Show resolved
Hide resolved
...-project/hadoop-hdfs-httpfs/src/main/java/org/apache/hadoop/fs/http/server/FSOperations.java
Outdated
Show resolved
Hide resolved
...c/main/java/org/apache/hadoop/hdfs/server/namenode/web/resources/NamenodeWebHdfsMethods.java
Outdated
Show resolved
Hide resolved
0a5997f to
d82af44
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
smengcl
left a comment
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. Thank you @virajjasani !
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
jojochuang
left a comment
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.
Sorry I am late to this. Found a potential issue.
| org.apache.hadoop.fs.Path path = | ||
| new org.apache.hadoop.fs.Path(fullpath); | ||
| boolean movedToTrash = Trash.moveToAppropriateTrash( | ||
| FileSystem.get(conf), path, conf); |
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.
This could lead to OOM. We should not create FileSystem object inside NameNode.
See https://issues.apache.org/jira/browse/HDFS-15052 for a similar problem.
0f6500f to
523d89e
Compare
523d89e to
3efb343
Compare
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
| // To avoid caching FS objects and prevent OOM issues | ||
| clonedConf.set("fs.hdfs.impl.disable.cache", "true"); | ||
| FileSystem fs = FileSystem.get(clonedConf); | ||
| boolean movedToTrash = Trash.moveToAppropriateTrash(fs, path, | ||
| clonedConf); |
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.
This is where I have tried to address @jojochuang's concerns reg how we can avoid causing OOM (not caching new FileSystem objects)
jojochuang
left a comment
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.
+1. I think this is good from a code perspective. Hard to tell if does address performance concern (cost of creating a Configuration and FileSystem object is not low). But in the worse case the feature can be turned off by a flag.
If it leaves room for improvement we can do so in a future release.
|
Agree, new Configuration and FileSystem object creation might cost bit of performance and more objects for GC. I will be on lookout for any other alternatives. Thanks for the reviews @jojochuang @smengcl @liuml07 |
|
Reverted from trunk, till the final conclusion is found |
…pache#2927) Reviewed-by: Siyao Meng <[email protected]>
…e trash (apache#2927)" This reverts commit 041488e.
https://issues.apache.org/jira/browse/HDFS-15982