mukul1987 commented on a change in pull request #2172:
URL: https://github.com/apache/hadoop/pull/2172#discussion_r462031404
##########
File path:
hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/FSDirRenameOp.java
##########
@@ -193,7 +194,7 @@ static INodesInPath unprotectedRenameTo(FSDirectory fsd,
}
validateNestSnapshot(fsd, src, dstParent.asDirectory(), snapshottableDirs);
-
+ FSDirSnapshotOp.checkUnderSameSnapshottableRoot(fsd, srcIIP, dstIIP);
Review comment:
There are 2 callers of fsd.ezManager.checkMoveValidity(srcIIP,
dstIIP);
##########
File path:
hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/FSDirSnapshotOp.java
##########
@@ -341,4 +341,24 @@ static void checkSnapshot(FSDirectory fsd, INodesInPath
iip,
checkSnapshot(iip.getLastINode(), snapshottableDirs);
}
}
+
+ static void checkUnderSameSnapshottableRoot(FSDirectory fsd,
+ INodesInPath srcIIP, INodesInPath dstIIP) throws IOException {
+ // Ensure rename out of a snapshottable root is not permitted if ordered
+ // snapshot deletion feature is enabled
+ if (fsd.isSnapshotDeletionOrdered()) {
+ SnapshotManager snapshotManager = fsd.getFSNamesystem().
+ getSnapshotManager();
+ String errMsg = "Source " + srcIIP.getPath() +
+ " and dest " + dstIIP.getPath() + " are not under " +
+ "the same snapshot root.";
+ INodeDirectory src = snapshotManager.
+ getSnapshottableAncestorDir(srcIIP);
+ INodeDirectory dst = snapshotManager.getSnapshottableAncestorDir(dstIIP);
+ if (!(dstIIP.isDescendant(snapshotManager.
+ getSnapshottableAncestorDir(srcIIP)))) {
Review comment:
we can replace snapshotManager.getSnapshottableAncestorDir(srcIIP))
with src here.
##########
File path:
hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/FSDirRenameOp.java
##########
@@ -193,7 +194,7 @@ static INodesInPath unprotectedRenameTo(FSDirectory fsd,
}
validateNestSnapshot(fsd, src, dstParent.asDirectory(), snapshottableDirs);
-
+ FSDirSnapshotOp.checkUnderSameSnapshottableRoot(fsd, srcIIP, dstIIP);
Review comment:
FSDirSnapshotOp.checkUnderSameSnapshottableRoot(fsd, srcIIP, dstIIP);
lets add snapshot rename check at the same places.
##########
File path:
hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/namenode/TestRenameWithOrderedSnapshotDeletion.java
##########
@@ -0,0 +1,96 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.hadoop.hdfs.server.namenode;
+
+import org.apache.hadoop.conf.Configuration;
+import org.apache.hadoop.fs.Path;
+import org.apache.hadoop.hdfs.DistributedFileSystem;
+import org.apache.hadoop.hdfs.MiniDFSCluster;
+import org.apache.hadoop.hdfs.DFSTestUtil;
+import org.junit.After;
+import org.junit.Before;
+import org.junit.Test;
+
+
+import java.io.IOException;
+
+import static org.apache.hadoop.hdfs.DFSConfigKeys.
+ DFS_NAMENODE_SNAPSHOT_DELETION_ORDERED;
+
+/**
+ * Test Rename with ordered snapshot deletion.
+ */
+public class TestRenameWithOrderedSnapshotDeletion {
+ private final Path snapshottableDir
+ = new Path("/" + getClass().getSimpleName());
+ private DistributedFileSystem hdfs;
+ private MiniDFSCluster cluster;
+
+ @Before
+ public void setUp() throws Exception {
+ final Configuration conf = new Configuration();
+ conf.setBoolean(DFS_NAMENODE_SNAPSHOT_DELETION_ORDERED, true);
+
+ cluster = new MiniDFSCluster.Builder(conf).numDataNodes(0).build();
+ cluster.waitActive();
+ hdfs = cluster.getFileSystem();
+ }
+
+ @After
+ public void tearDown() throws Exception {
+ if (cluster != null) {
+ cluster.shutdown();
+ cluster = null;
+ }
+ }
+
+ @Test(timeout = 60000)
+ public void testRename() throws Exception {
+ final Path dir1 = new Path("/dir1");
+ final Path dir2 = new Path("/dir2");
+ final Path sub0 = new Path(snapshottableDir, "sub0");
+ hdfs.mkdirs(sub0);
+ final Path file1 = new Path(dir1, "file1");
+ final Path file2 = new Path(sub0, "file2");
+ hdfs.mkdirs(snapshottableDir);
+ hdfs.mkdirs(dir1);
+ hdfs.mkdirs(dir2);
+ hdfs.mkdirs(sub0);
+ DFSTestUtil.createFile(hdfs, file1, 0, (short) 1, 0);
+ DFSTestUtil.createFile(hdfs, file2, 0, (short) 1, 0);
+ hdfs.allowSnapshot(snapshottableDir);
+ // rename from non snapshottable dir to snapshottable dir should fail
+ validateRename(file1, sub0);
+ hdfs.createSnapshot(snapshottableDir, "s0");
+ validateRename(file1, sub0);
+ // rename across non snapshottable dirs should work
+ hdfs.rename(file1, dir2);
+ // rename beyond snapshottable root should fail
+ validateRename(file2, dir1);
+ // rename within snapshottable root should work
+ hdfs.rename(file2, snapshottableDir);
Review comment:
Lets also add some directory level renames here
##########
File path:
hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/namenode/TestRenameWithOrderedSnapshotDeletion.java
##########
@@ -0,0 +1,96 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.hadoop.hdfs.server.namenode;
+
+import org.apache.hadoop.conf.Configuration;
+import org.apache.hadoop.fs.Path;
+import org.apache.hadoop.hdfs.DistributedFileSystem;
+import org.apache.hadoop.hdfs.MiniDFSCluster;
+import org.apache.hadoop.hdfs.DFSTestUtil;
+import org.junit.After;
+import org.junit.Before;
+import org.junit.Test;
+
+
+import java.io.IOException;
+
+import static org.apache.hadoop.hdfs.DFSConfigKeys.
+ DFS_NAMENODE_SNAPSHOT_DELETION_ORDERED;
+
+/**
+ * Test Rename with ordered snapshot deletion.
+ */
+public class TestRenameWithOrderedSnapshotDeletion {
+ private final Path snapshottableDir
+ = new Path("/" + getClass().getSimpleName());
+ private DistributedFileSystem hdfs;
+ private MiniDFSCluster cluster;
+
+ @Before
+ public void setUp() throws Exception {
+ final Configuration conf = new Configuration();
+ conf.setBoolean(DFS_NAMENODE_SNAPSHOT_DELETION_ORDERED, true);
+
+ cluster = new MiniDFSCluster.Builder(conf).numDataNodes(0).build();
+ cluster.waitActive();
+ hdfs = cluster.getFileSystem();
+ }
+
+ @After
+ public void tearDown() throws Exception {
+ if (cluster != null) {
+ cluster.shutdown();
+ cluster = null;
+ }
+ }
+
+ @Test(timeout = 60000)
+ public void testRename() throws Exception {
+ final Path dir1 = new Path("/dir1");
+ final Path dir2 = new Path("/dir2");
+ final Path sub0 = new Path(snapshottableDir, "sub0");
+ hdfs.mkdirs(sub0);
+ final Path file1 = new Path(dir1, "file1");
+ final Path file2 = new Path(sub0, "file2");
+ hdfs.mkdirs(snapshottableDir);
+ hdfs.mkdirs(dir1);
+ hdfs.mkdirs(dir2);
+ hdfs.mkdirs(sub0);
+ DFSTestUtil.createFile(hdfs, file1, 0, (short) 1, 0);
+ DFSTestUtil.createFile(hdfs, file2, 0, (short) 1, 0);
+ hdfs.allowSnapshot(snapshottableDir);
+ // rename from non snapshottable dir to snapshottable dir should fail
+ validateRename(file1, sub0);
+ hdfs.createSnapshot(snapshottableDir, "s0");
+ validateRename(file1, sub0);
+ // rename across non snapshottable dirs should work
+ hdfs.rename(file1, dir2);
+ // rename beyond snapshottable root should fail
+ validateRename(file2, dir1);
+ // rename within snapshottable root should work
+ hdfs.rename(file2, snapshottableDir);
+ }
+
+ private void validateRename(Path src, Path dest) {
+ try {
+ hdfs.rename(src, dest);
+ } catch (IOException ioe) {
+ ioe.getMessage().contains("are not under the same snapshot root.");
Review comment:
Add Assert.equals or Assert true here ?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
