HDFS-14925. Rename operation should check nest snapshot #1670
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
|
@manojpec, please help to review this patch :) |
|
@bshashikant could you help review this PR? |
|
💔 -1 overall
This message was automatically generated. |
dineshchitlangia
requested changes
Oct 23, 2019
...-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/FSDirRenameOp.java
Outdated
Show resolved
Hide resolved
|
This patch misses considering dir to dir overwrite scenario, will modify and push later, thx. |
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
TestRenameWithSnapshots is unrelated, due to HDFS-14910, but please check TestRandomOpsWithSnapshots |
|
@jojochuang can you pls tell me how can I run the unittest locally, I see the jenkins job runs but I couldn't run this on my centos server |
|
If you have editors like IntelliJ, you can run any of the tests. Double check for the failed tests. Sometimes they are flaky tests and we are okay to +1 if they don't reproduce all the time. |
If the src directory or any of its descendant is snapshottable and the dst directory or any of its ancestors is snapshottable, we consider this as nested snapshot, which should be denied. V2: 1. reduce duplicate code 2. take `directory rename overwrite` into consideration V3: 1. fix unit test fail Signed-off-by: Zhao Junwang <zhjwpku@gmail.com>
|
💔 -1 overall
This message was automatically generated. |
|
@jojochuang I didn't get TestRollingUpgrade failed when I run it locally. Can you pls help to check is it a flaky test? Following it error message: |
jojochuang
requested changes
Oct 26, 2019
...-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/FSDirRenameOp.java
Outdated
Show resolved
Hide resolved
...doop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/snapshot/SnapshotManager.java
Show resolved
Hide resolved
...-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/FSDirRenameOp.java
Outdated
Show resolved
Hide resolved
Signed-off-by: Zhao Junwang <zhjwpku@gmail.com>
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
Signed-off-by: Zhao Junwang <zhjwpku@gmail.com>
|
🎊 +1 overall
This message was automatically generated. |
|
@jojochuang @dineshchitlangia finally hadoop-yetus report |
jojochuang
requested changes
Oct 29, 2019
...s/src/test/java/org/apache/hadoop/hdfs/server/namenode/snapshot/TestRenameWithSnapshots.java
Show resolved
Hide resolved
jojochuang
reviewed
Oct 29, 2019
| * src) which do not have snapshots yet. If this list is not empty, that | ||
| * means rename src has snapshottable descendant directories. | ||
| */ | ||
| if (snapshottableDirs != null && snapshottableDirs.size() > 0) { |
There was a problem hiding this comment.
I was concerned about the case where the source already has a snapshot.
But turns out that case is already handled before this patch with the following exception
The directory /dir1/foo cannot be deleted since /dir1/foo is snapshottable and already has snapshots at org.apache.hadoop.hdfs.server.namenode.FSDirSnapshotOp.checkSnapshot(FSDirSnapshotOp.java:297)
Signed-off-by: Zhao Junwang <zhjwpku@gmail.com>
|
💔 -1 overall
This message was automatically generated. |
|
The changes look good to me as well. +1 |
|
Thanks @bshashikant for review and @zhjwpku for the patch! |
asfgit
pushed a commit
that referenced
this pull request
Nov 1, 2019
If the src directory or any of its descendant is snapshottable and the dst directory or any of its ancestors is snapshottable, we consider this as nested snapshot, which should be denied. Reviewed-by: Shashikant Banerjee <shashikant@apache.org> (cherry picked from commit de6b8b0) (cherry picked from commit c9fc118)
asfgit
pushed a commit
that referenced
this pull request
Nov 1, 2019
If the src directory or any of its descendant is snapshottable and the dst directory or any of its ancestors is snapshottable, we consider this as nested snapshot, which should be denied. Reviewed-by: Shashikant Banerjee <shashikant@apache.org> (cherry picked from commit de6b8b0)
RogPodge
pushed a commit
to RogPodge/hadoop
that referenced
this pull request
Mar 25, 2020
If the src directory or any of its descendant is snapshottable and the dst directory or any of its ancestors is snapshottable, we consider this as nested snapshot, which should be denied. Reviewed-by: Shashikant Banerjee <shashikant@apache.org>
bentito
pushed a commit
to bentito/hadoop
that referenced
this pull request
Dec 2, 2020
If the src directory or any of its descendant is snapshottable and the dst directory or any of its ancestors is snapshottable, we consider this as nested snapshot, which should be denied. Reviewed-by: Shashikant Banerjee <shashikant@apache.org> (cherry picked from commit de6b8b0) (cherry picked from commit c9fc118)
bentito
pushed a commit
to bentito/hadoop
that referenced
this pull request
Dec 3, 2020
If the src directory or any of its descendant is snapshottable and the dst directory or any of its ancestors is snapshottable, we consider this as nested snapshot, which should be denied. Reviewed-by: Shashikant Banerjee <shashikant@apache.org> (cherry picked from commit de6b8b0) (cherry picked from commit c9fc118)
jojochuang
added a commit
to jojochuang/hadoop
that referenced
this pull request
May 23, 2023
If the src directory or any of its descendant is snapshottable and the dst directory or any of its ancestors is snapshottable, we consider this as nested snapshot, which should be denied. Reviewed-by: Shashikant Banerjee <shashikant@apache.org> (cherry picked from commit de6b8b0) (cherry picked from commit c9fc118) (cherry picked from commit cda8b2a) Change-Id: I5f3b2ee646e5ac29836b589fa317f72f2c023a11
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.
When we do rename operation for directory,
if the src directory or any of its descendant is snapshottable
and the dst directory or its any of its ancestors is snapshottable,
we consider this as nested snapshot, which should be denied.
V2:
1. reduce duplicate code
2. take
directory rename overwriteinto considerationSigned-off-by: Zhao Junwang zhjwpku@gmail.com