From 6ef6594c7ee09b561e42c16ce4e91c0479908ad8 Mon Sep 17 00:00:00 2001 From: Wei-Chiu Chuang Date: Tue, 1 Oct 2019 08:46:46 -0700 Subject: [PATCH] HDFS-14492. Snapshot memory leak. Contributed by Wei-Chiu Chuang. (#1370) * HDFS-14492. Snapshot memory leak. Contributed by Wei-Chiu Chuang. Change-Id: I9e5e450c07ad70aa1905973896c4f627042dbd37 * Fix checkstyle Change-Id: I16d4bd4f03a971e1ed36cf57d89dc42357ef8fbf --- .../hdfs/server/namenode/INodeDirectory.java | 6 ++++++ .../hadoop/hdfs/server/namenode/INodeFile.java | 3 +++ .../namenode/snapshot/AbstractINodeDiffList.java | 4 ++++ .../snapshot/TestRenameWithSnapshots.java | 15 +-------------- .../namenode/snapshot/TestSnapshotDeletion.java | 2 +- 5 files changed, 15 insertions(+), 15 deletions(-) diff --git a/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/INodeDirectory.java b/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/INodeDirectory.java index e71cb0a067..b592c3ff89 100644 --- a/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/INodeDirectory.java +++ b/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/INodeDirectory.java @@ -838,6 +838,12 @@ public void cleanSubtree(ReclaimContext reclaimContext, final int snapshotId, // there is snapshot data if (sf != null) { sf.cleanDirectory(reclaimContext, this, snapshotId, priorSnapshotId); + // If the inode has empty diff list and sf is not a + // DirectorySnapshottableFeature, remove the feature to save heap. + if (sf.getDiffs().isEmpty() && + !(sf instanceof DirectorySnapshottableFeature)) { + this.removeFeature(sf); + } } else { // there is no snapshot data if (priorSnapshotId == Snapshot.NO_SNAPSHOT_ID && diff --git a/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/INodeFile.java b/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/INodeFile.java index 7b6f1e349a..ce654b789f 100644 --- a/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/INodeFile.java +++ b/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/INodeFile.java @@ -744,6 +744,9 @@ public void cleanSubtree(ReclaimContext reclaimContext, sf.cleanFile(reclaimContext, this, snapshot, priorSnapshotId, getStoragePolicyID()); updateRemovedUnderConstructionFiles(reclaimContext); + if (sf.getDiffs().isEmpty()) { + this.removeFeature(sf); + } } else { if (snapshot == CURRENT_STATE_ID) { if (priorSnapshotId == NO_SNAPSHOT_ID) { diff --git a/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/snapshot/AbstractINodeDiffList.java b/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/snapshot/AbstractINodeDiffList.java index 9cd87f1126..163b181ed1 100644 --- a/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/snapshot/AbstractINodeDiffList.java +++ b/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/snapshot/AbstractINodeDiffList.java @@ -45,6 +45,10 @@ public final DiffList asList() { return diffs != null ? DiffList.unmodifiableList(diffs) : DiffList.emptyList(); } + + public boolean isEmpty() { + return diffs == null || diffs.isEmpty(); + } /** Clear the list. */ public void clear() { diff --git a/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/namenode/snapshot/TestRenameWithSnapshots.java b/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/namenode/snapshot/TestRenameWithSnapshots.java index db3d93fd53..b8898eb3e8 100644 --- a/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/namenode/snapshot/TestRenameWithSnapshots.java +++ b/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/namenode/snapshot/TestRenameWithSnapshots.java @@ -2144,24 +2144,11 @@ public void testRenameDirAndDeleteSnapshot_6() throws Exception { INodeDirectory dir2Node = fsdir.getINode4Write(dir2.toString()) .asDirectory(); assertTrue("the diff list of " + dir2 - + " should be empty after deleting s0", dir2Node.getDiffs().asList() - .isEmpty()); + + " should be empty after deleting s0", !dir2Node.isWithSnapshot()); assertTrue(hdfs.exists(newfoo)); INode fooRefNode = fsdir.getINode4Write(newfoo.toString()); assertTrue(fooRefNode instanceof INodeReference.DstReference); - INodeDirectory fooNode = fooRefNode.asDirectory(); - // fooNode should be still INodeDirectory (With Snapshot) since we call - // recordModification before the rename - assertTrue(fooNode.isWithSnapshot()); - assertTrue(fooNode.getDiffs().asList().isEmpty()); - INodeDirectory barNode = fooNode.getChildrenList(Snapshot.CURRENT_STATE_ID) - .get(0).asDirectory(); - // bar should also be INodeDirectory (With Snapshot), and both of its diff - // list and children list are empty - assertTrue(barNode.getDiffs().asList().isEmpty()); - assertTrue(barNode.getChildrenList(Snapshot.CURRENT_STATE_ID).isEmpty()); - restartClusterAndCheckImage(true); } diff --git a/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/namenode/snapshot/TestSnapshotDeletion.java b/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/namenode/snapshot/TestSnapshotDeletion.java index 8bd796718c..3e318b3a30 100644 --- a/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/namenode/snapshot/TestSnapshotDeletion.java +++ b/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/namenode/snapshot/TestSnapshotDeletion.java @@ -527,7 +527,7 @@ public void testDeleteEarliestSnapshot2() throws Exception { assertEquals(snapshot1.getId(), diffList.getLast().getSnapshotId()); diffList = fsdir.getINode(metaChangeDir.toString()).asDirectory() .getDiffs(); - assertEquals(0, diffList.asList().size()); + assertEquals(null, diffList); // check 2. noChangeDir and noChangeFile are still there final INodeDirectory noChangeDirNode =