fix: snapshot scenarios & checkstyle

Author: huangzhaobo99

hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/FSDirRenameOp.java

@@ -82,15 +82,14 @@ private static Triple<Boolean, Optional<QuotaCounts>, Optional<QuotaCounts>> ver
       return Triple.of(false, srcDelta, dstDelta);
     }
 
-    // Verify srcPath and dstPath without valid 'DirectoryWithQuotaFeature'
-    // Note: In overwrite scenarios, quota calculation is still required because:
-    // Overwrite operations delete existing content (affecting rootDir's quota)
-    if (FSDirectory.verifyPathWithoutValidQuotaFeature(src, src.length() - 1)
-        && FSDirectory.verifyPathWithoutValidQuotaFeature(dst, dst.length() - 1) 
-        && !overwrite) {
+    // Verify path without valid 'DirectoryWithQuotaFeature'
+    // Note: In overwrite scenarios, quota calculation is still required,
+    // overwrite operations delete existing content, which affects the root directory's quota.
+    if (!overwrite && FSDirectory.verifyPathWithoutValidFeature(src)
+        && FSDirectory.verifyPathWithoutValidFeature(dst)) {
       return Triple.of(false, srcDelta, dstDelta);
     }
-    
+
     int i = 0;
     while (src.getINode(i) == dst.getINode(i)) {
       i++;
@@ -723,8 +722,8 @@ private static class RenameOperation {
       } else {
         withCount = null;
       }
-      this.updateQuota = quotaPair.getLeft();
       // Set quota for src and dst, ignore src is in Snapshot or is Reference
+      this.updateQuota = quotaPair.getLeft() || isSrcInSnapshot || srcChildIsReference;
       this.srcSubTreeCount = withCount == null ?
           quotaPair.getMiddle() : Optional.empty();
       this.dstSubTreeCount = quotaPair.getRight();
@@ -768,9 +767,9 @@ long removeSrc() throws IOException {
         throw new IOException(error);
       } else {
         // update the quota count if necessary
-        Optional<QuotaCounts> countOp = sameStoragePolicy ?
-            srcSubTreeCount : Optional.empty();
         if (updateQuota) {
+          Optional<QuotaCounts> countOp = sameStoragePolicy ?
+              srcSubTreeCount : Optional.empty();
           fsd.updateCountForDelete(srcChild, srcIIP, countOp);
         }
         srcIIP = INodesInPath.replace(srcIIP, srcIIP.length() - 1, null);
@@ -787,9 +786,9 @@ boolean removeSrc4OldRename() {
         return false;
       } else {
         // update the quota count if necessary
-        Optional<QuotaCounts> countOp = sameStoragePolicy ?
-            srcSubTreeCount : Optional.empty();
         if (updateQuota) {
+          Optional<QuotaCounts> countOp = sameStoragePolicy ?
+              srcSubTreeCount : Optional.empty();
           fsd.updateCountForDelete(srcChild, srcIIP, countOp);
         }
         srcIIP = INodesInPath.replace(srcIIP, srcIIP.length() - 1, null);

hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/FSDirectory.java

@@ -1241,22 +1241,19 @@ static void verifyQuota(INodesInPath iip, int pos, QuotaCounts deltas,
       }
     }
   }
-  
+
   /**
-   * Verifies that the path from the specified position to the root
+   * Verify that the path from the specified position to the root
    * (excluding the root itself) does not contain any valid quota features.
    *
    * @param iip the INodesInPath containing all the ancestral INodes.
-   * @param pos Starting position in the path (0-based index).
-   * @return true if no valid quota features are found along the path (root excluded),
-   * false if any directory in the path has an active quota feature.
+   * @return true if no valid features are found along the path,
+   * false if any directory in the path has an active feature.
    */
-  static boolean verifyPathWithoutValidQuotaFeature(INodesInPath iip, int pos) {
-    // Does not include the root path
-    for (int i = Math.min(pos, iip.length()) - 1; i >= 1; i--) {
-      final DirectoryWithQuotaFeature q =
-          iip.getINode(i).asDirectory().getDirectoryWithQuotaFeature();
-      if (q != null) {
+  static boolean verifyPathWithoutValidFeature(INodesInPath iip) {
+    for (int i = iip.length() - 2; i >= 1; i--) {
+      INodeDirectory d = iip.getINode(i).asDirectory();
+      if (d.isWithQuota()) {
         return false;
       }
     }
@@ -1364,11 +1361,12 @@ private void copyINodeDefaultAcl(INode child, FsPermission modes) {
    * @param inode the new INode to add
    * @param modes create modes
    * @param checkQuota whether to check quota
+   * @param updateQuota whether to update quota
    * @return an INodesInPath instance containing the new INode
    */
   @VisibleForTesting
-  public INodesInPath addLastINode(INodesInPath existing, INode inode,
-      FsPermission modes, boolean checkQuota, Optional<QuotaCounts> quotaCount, boolean updateQuota)
+  public INodesInPath addLastINode(INodesInPath existing, INode inode, FsPermission modes,
+      boolean checkQuota, Optional<QuotaCounts> quotaCount, boolean updateQuota)
       throws QuotaExceededException {
     assert existing.getLastINode() != null &&
         existing.getLastINode().isDirectory();
@@ -1400,9 +1398,8 @@ public INodesInPath addLastINode(INodesInPath existing, INode inode,
     // always verify inode name
     verifyINodeName(inode.getLocalNameBytes());
 
-    QuotaCounts counts;
     if (updateQuota) {
-      counts = quotaCount.orElseGet(() -> inode.
+      QuotaCounts counts = quotaCount.orElseGet(() -> inode.
           computeQuotaUsage(getBlockStoragePolicySuite(),
           parent.getStoragePolicyID(), false,
           Snapshot.CURRENT_STATE_ID));
@@ -1413,7 +1410,7 @@ public INodesInPath addLastINode(INodesInPath existing, INode inode,
     final boolean added = parent.addChild(inode, true,
         existing.getLatestSnapshotId());
     if (!added && updateQuota) {
-      counts = quotaCount.orElseGet(() -> inode.
+      QuotaCounts counts = quotaCount.orElseGet(() -> inode.
           computeQuotaUsage(getBlockStoragePolicySuite(),
           parent.getStoragePolicyID(), false,
           Snapshot.CURRENT_STATE_ID));

hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/namenode/TestCorrectnessOfQuotaAfterRenameOp.java

@@ -80,7 +80,7 @@ public void testQuotaUsageWhenRenameWithSameStoragePolicy() throws Exception {
     DFSTestUtil.createFile(dfs, file2, fileLen, replication, 0);
 
     final Path dstDir1 = new Path(testParentDir2, "dst-dir");
-    // If dstDir1 not exist, after the rename operation, 
+    // If dstDir1 not exist, after the rename operation,
     // the root dir's quota usage should remain unchanged.
     QuotaUsage quotaUsage1 = dfs.getQuotaUsage(new Path("/"));
     ContentSummary cs1 = dfs.getContentSummary(testParentDir1);
@@ -169,10 +169,10 @@ public void testQuotaUsageWhenRenameWithDifferStoragePolicy() throws Exception {
   }
 
   @Test
-  public void testRenameWithoutValidQuotaFeature() throws Exception {
+  public void testRenameWithoutValidFeature() throws Exception {
     final int fileLen = 1024;
     final short replication = 3;
-    final Path root = new Path("/testRoot");
+    final Path root = new Path("/testRename");
     assertTrue(dfs.mkdirs(root));
 
     Path testParentDir1 = new Path(root, "testDir1");
@@ -189,36 +189,38 @@ public void testRenameWithoutValidQuotaFeature() throws Exception {
     }
 
     // 1. Test rename1
-    QuotaUsage quotaUsage1 = dfs.getQuotaUsage(new Path("/"));
-    final Path dstDir1 = new Path(testParentDir2, "dst-dir");
-    ContentSummary cs1 = dfs.getContentSummary(testParentDir1);
-
-    // srcDir=/testRoot/testDir1/src-dir
-    // dstDir=/testRoot/testDir2/dst-dir dstDir1 not exist
-    boolean rename = dfs.rename(srcDir, dstDir1);
-    assertTrue(rename);
-    ContentSummary cs2 = dfs.getContentSummary(testParentDir2);
-    assertEquals(cs1, cs2);
-
-    QuotaUsage quotaUsage2 = dfs.getQuotaUsage(new Path("/"));
-    assertEquals(quotaUsage1.getFileAndDirectoryCount(), quotaUsage2.getFileAndDirectoryCount());
+    ContentSummary rootContentSummary1 = dfs.getContentSummary(new Path("/"));
+    QuotaUsage rootQuotaUsage1 = dfs.getQuotaUsage(new Path("/"));
+    ContentSummary contentSummary1 = dfs.getContentSummary(testParentDir1);
+    // srcDir=/testRename/testDir1/src-dir
+    // dstDir=/testRename/testDir2/dst-dir dstDir1 not exist
+    final Path dstDir2 = new Path(testParentDir2, "dst-dir");
+    assertTrue(dfs.rename(srcDir, dstDir2));
+    ContentSummary contentSummary2 = dfs.getContentSummary(testParentDir2);
+    assertEquals(contentSummary1, contentSummary2);
+    QuotaUsage rootQuotaUsage2 = dfs.getQuotaUsage(new Path("/"));
+    assertEquals(rootQuotaUsage1.getFileAndDirectoryCount(),
+        rootQuotaUsage2.getFileAndDirectoryCount());
+    // The return values of the getContentSummary() and getQuotaUsage() should be consistent
+    assertEquals(rootContentSummary1.getFileAndDirectoryCount(),
+        rootQuotaUsage2.getFileAndDirectoryCount());
 
     // 2. Test rename2
-    final Path dstDir2 = new Path(testParentDir3, "dst-dir");
-    assertTrue(dfs.mkdirs(dstDir2));
-    long originDstDir2Usage = dfs.getQuotaUsage(dstDir2).getFileAndDirectoryCount();
-    // The usage for covering the root dir should not include dstDir2 usage
+    final Path dstDir3 = new Path(testParentDir3, "dst-dir");
+    assertTrue(dfs.mkdirs(dstDir3));
+    long originDstDir2Usage = dfs.getQuotaUsage(dstDir3).getFileAndDirectoryCount();
+    // Exclude dstDir2 usage
     long rootINodeCount1 =
         dfs.getQuotaUsage(new Path("/")).getFileAndDirectoryCount() - originDstDir2Usage;
-    ContentSummary cs3 = dfs.getContentSummary(testParentDir2);
+    ContentSummary contentSummary3 = dfs.getContentSummary(testParentDir2);
 
     // Src and dst must be same (all file or all dir)
-    // dstDir1=/testRoot/testDir3/dst-dir
-    // dstDir2=/testRoot/testDir3/dst-dir
-    dfs.rename(dstDir1, dstDir2, Options.Rename.OVERWRITE);
+    // dstDir2=/testRename/testDir3/dst-dir
+    // dstDir3=/testRename/testDir3/dst-dir
+    dfs.rename(dstDir2, dstDir3, Options.Rename.OVERWRITE);
     long rootINodeCount2 = dfs.getQuotaUsage(new Path("/")).getFileAndDirectoryCount();
     assertEquals(rootINodeCount1, rootINodeCount2);
-    ContentSummary cs4 = dfs.getContentSummary(testParentDir3);
-    assertEquals(cs3, cs4);
+    ContentSummary contentSummary4 = dfs.getContentSummary(testParentDir3);
+    assertEquals(contentSummary3, contentSummary4);
   }
 }