From 55c77bf722f2b6fcde135c0f71454647a8d2a3db Mon Sep 17 00:00:00 2001 From: Tsz-Wo Nicholas Sze Date: Tue, 27 Feb 2018 15:28:41 -0800 Subject: [PATCH] HDFS-13143. SnapshotDiff - snapshotDiffReport might be inconsistent if the snapshotDiff calculation happens between a snapshot and the current tree. Contributed by Shashikant Banerjee --- .../org/apache/hadoop/hdfs/DFSClient.java | 15 ++++++++++++ .../hadoop/hdfs/DistributedFileSystem.java | 23 +++++++++++++++++-- .../hadoop/hdfs/protocol/ClientProtocol.java | 3 +-- .../snapshot/TestSnapshotDiffReport.java | 13 +++++++++++ 4 files changed, 50 insertions(+), 4 deletions(-) diff --git a/hadoop-hdfs-project/hadoop-hdfs-client/src/main/java/org/apache/hadoop/hdfs/DFSClient.java b/hadoop-hdfs-project/hadoop-hdfs-client/src/main/java/org/apache/hadoop/hdfs/DFSClient.java index 2497c40106..af7b540043 100644 --- a/hadoop-hdfs-project/hadoop-hdfs-client/src/main/java/org/apache/hadoop/hdfs/DFSClient.java +++ b/hadoop-hdfs-project/hadoop-hdfs-client/src/main/java/org/apache/hadoop/hdfs/DFSClient.java @@ -139,6 +139,7 @@ import org.apache.hadoop.hdfs.protocol.SnapshottableDirectoryStatus; import org.apache.hadoop.hdfs.protocol.UnresolvedPathException; import org.apache.hadoop.hdfs.protocol.ZoneReencryptionStatus; +import org.apache.hadoop.hdfs.protocol.SnapshotDiffReport; import org.apache.hadoop.hdfs.protocol.SnapshotDiffReportListing; import org.apache.hadoop.hdfs.protocol.datatransfer.DataTransferProtoUtil; import org.apache.hadoop.hdfs.protocol.datatransfer.IOStreamPair; @@ -2118,6 +2119,20 @@ public void disallowSnapshot(String snapshotRoot) throws IOException { /** * Get the difference between two snapshots, or between a snapshot and the * current tree of a directory. + * @see ClientProtocol#getSnapshotDiffReport + */ + public SnapshotDiffReport getSnapshotDiffReport(String snapshotDir, + String fromSnapshot, String toSnapshot) throws IOException { + checkOpen(); + try (TraceScope ignored = tracer.newScope("getSnapshotDiffReport")) { + return namenode + .getSnapshotDiffReport(snapshotDir, fromSnapshot, toSnapshot); + } catch (RemoteException re) { + throw re.unwrapRemoteException(); + } + } + /** + * Get the difference between two snapshots of a directory iteratively. * @see ClientProtocol#getSnapshotDiffReportListing */ public SnapshotDiffReportListing getSnapshotDiffReportListing( diff --git a/hadoop-hdfs-project/hadoop-hdfs-client/src/main/java/org/apache/hadoop/hdfs/DistributedFileSystem.java b/hadoop-hdfs-project/hadoop-hdfs-client/src/main/java/org/apache/hadoop/hdfs/DistributedFileSystem.java index 35b64173a7..fe2a5653bb 100644 --- a/hadoop-hdfs-project/hadoop-hdfs-client/src/main/java/org/apache/hadoop/hdfs/DistributedFileSystem.java +++ b/hadoop-hdfs-project/hadoop-hdfs-client/src/main/java/org/apache/hadoop/hdfs/DistributedFileSystem.java @@ -2020,8 +2020,13 @@ snapshotDiffReportListingRemoteIterator( @Override public RemoteIterator doCall(final Path p) throws IOException { - return new SnapshotDiffReportListingIterator( - getPathName(p), fromSnapshot, toSnapshot); + if (!isValidSnapshotName(fromSnapshot) || !isValidSnapshotName( + toSnapshot)) { + throw new UnsupportedOperationException("Remote Iterator is" + + "supported for snapshotDiffReport between two snapshots"); + } + return new SnapshotDiffReportListingIterator(getPathName(p), + fromSnapshot, toSnapshot); } @Override @@ -2081,9 +2086,23 @@ public SnapshotDiffReportListing next() throws IOException { } } + private boolean isValidSnapshotName(String snapshotName) { + // If any of the snapshots specified in the getSnapshotDiffReport call + // is null or empty, it points to the current tree. + return (snapshotName != null && !snapshotName.isEmpty()); + } + private SnapshotDiffReport getSnapshotDiffReportInternal( final String snapshotDir, final String fromSnapshot, final String toSnapshot) throws IOException { + // In case the diff needs to be computed between a snapshot and the current + // tree, we should not do iterative diffReport computation as the iterative + // approach might fail if in between the rpc calls the current tree + // changes in absence of the global fsn lock. + if (!isValidSnapshotName(fromSnapshot) || !isValidSnapshotName( + toSnapshot)) { + return dfs.getSnapshotDiffReport(snapshotDir, fromSnapshot, toSnapshot); + } byte[] startPath = DFSUtilClient.EMPTY_BYTES; int index = -1; SnapshotDiffReportGenerator snapshotDiffReport; diff --git a/hadoop-hdfs-project/hadoop-hdfs-client/src/main/java/org/apache/hadoop/hdfs/protocol/ClientProtocol.java b/hadoop-hdfs-project/hadoop-hdfs-client/src/main/java/org/apache/hadoop/hdfs/protocol/ClientProtocol.java index 0d77037d9d..f5d5e8257e 100644 --- a/hadoop-hdfs-project/hadoop-hdfs-client/src/main/java/org/apache/hadoop/hdfs/protocol/ClientProtocol.java +++ b/hadoop-hdfs-project/hadoop-hdfs-client/src/main/java/org/apache/hadoop/hdfs/protocol/ClientProtocol.java @@ -1308,8 +1308,7 @@ SnapshotDiffReport getSnapshotDiffReport(String snapshotRoot, String fromSnapshot, String toSnapshot) throws IOException; /** - * Get the difference between two snapshots, or between a snapshot and the - * current tree of a directory. + * Get the difference between two snapshots of a directory iteratively. * * @param snapshotRoot * full path of the directory where snapshots are taken diff --git a/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/namenode/snapshot/TestSnapshotDiffReport.java b/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/namenode/snapshot/TestSnapshotDiffReport.java index 3bfcfbf971..47677e0edc 100644 --- a/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/namenode/snapshot/TestSnapshotDiffReport.java +++ b/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/namenode/snapshot/TestSnapshotDiffReport.java @@ -1539,4 +1539,17 @@ public void testSnapshotDiffReportRemoteIterator() throws Exception { new DiffReportEntry(DiffType.DELETE, DFSUtil.string2Bytes("dir3/file3"))); } + + @Test + public void testSnapshotDiffReportRemoteIterator2() throws Exception { + final Path root = new Path("/"); + hdfs.mkdirs(root); + SnapshotTestHelper.createSnapshot(hdfs, root, "s0"); + try { + hdfs.snapshotDiffReportListingRemoteIterator(root, "s0", ""); + } catch (Exception e) { + Assert.assertTrue(e.getMessage().contains("Remote Iterator is" + + "supported for snapshotDiffReport between two snapshots")); + } + } }