HDFS-15488. Add a command to list all snapshots for a snaphottable root with snapshot Ids. #2166
Conversation
This file contains 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
|
💔 -1 overall
This message was automatically generated. |
ayushtkn
reviewed
Jul 23, 2020
There was a problem hiding this comment.
Thanx @bshashikant for the work here. Had a very quick look, so might have missed stuffs(spare me for that, if so :-) ) mostly looks good.
Couple of stuffs give a check, Jenkins too has some complains, that you need to sort as well
| */ | ||
| public SnapshotStatus[] getSnapshotListing(Path snapshotRoot) | ||
| throws IOException { | ||
| return dfs.getSnapshotListing(getPathName(snapshotRoot)); |
There was a problem hiding this comment.
- Should relative path be resolved as well?
Path absF = fixRelativePart(path); - Should Increment Read Statistics as well.
statistics.incrementReadOps(1);
| * | ||
| * @return Information about all the snapshots for a snapshottable directory | ||
| * @throws IOException If an I/O error occurred | ||
| * @see ClientProtocol#getSnapshotListing() |
There was a problem hiding this comment.
Should be :
ClientProtocol#getSnapshotListing(String)
| public SnapshotStatus[] getSnapshotListing(String snapshotRoot) | ||
| throws IOException { | ||
| checkOpen(); | ||
| try (TraceScope ignored = tracer.newScope("getSnapshottableDirListing")) { |
There was a problem hiding this comment.
Seems Copy-paste error, Change to
try (TraceScope ignored = tracer.newScope("getSnapshotListing")) {
| @@ -2004,6 +2005,16 @@ public void renameSnapshot(String snapshotRoot, String snapshotOldName, | |||
| return status; | |||
| } | |||
|
|
|||
| @Override // Client Protocol | |||
| public SnapshotStatus[] getSnapshotListing(String path) | |||
There was a problem hiding this comment.
Keep the argument name consistent, In ClientProtocol & Router its snapshotRoot, better keep same everywhere
| snapshotRoot); | ||
| success = true; | ||
| } catch (AccessControlException ace) { | ||
| logAuditEvent(success, "listSnapshots", null, null, null); |
There was a problem hiding this comment.
Should put the path as well in the audit log too
logAuditEvent(success, "listSnapshots", snapshotRoot);
...hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/tools/snapshot/LsSnapshot.java
Show resolved
Hide resolved
Comment on lines
50
to
56
| FileSystem fs = FileSystem.get(getConf()); | ||
| if (! (fs instanceof DistributedFileSystem)) { | ||
| System.err.println( | ||
| "lsSnapshot can only be used in DistributedFileSystem"); | ||
| return 1; | ||
| } | ||
| DistributedFileSystem dfs = (DistributedFileSystem) fs; |
There was a problem hiding this comment.
Instead can use
DistributedFileSystem dfs = AdminHelper.getDFS(getConf());
This shall handle ViewFsOverloadScheme as well.
This would throw IllegalArgumentException so it should be inside the try block
Comment on lines
62
to
55
| } catch (IOException e) { | ||
| String[] content = e.getLocalizedMessage().split("\n"); | ||
| System.err.println("lsSnapshot: " + content[0]); | ||
| e.printStackTrace(System.err); | ||
| return 1; |
There was a problem hiding this comment.
I don't think we need to print the stack trace on the CLI? Just a line of error should work? Mostly it should trigger for FNF or for SnapshotException if the directory is not snapshottable. if required we can have the exception with trace in the debug log
Apart we should catch Exception as well for any runtime exceptions, propagating the exception in CLI won't look good
| */ | ||
| @Test(timeout = 60000) | ||
| public void testListSnapshot() throws Exception { | ||
| cluster.getNamesystem().getSnapshotManager().setAllowNestedSnapshots(true); |
There was a problem hiding this comment.
fsn is already there, can change to:
fsn.getSnapshotManager().setAllowNestedSnapshots(true);
Comment on lines
76
to
81
| try { | ||
| hdfs.getSnapshotListing(dir1); | ||
| } catch (Exception e) { | ||
| assertTrue(e.getMessage().contains( | ||
| "Directory is not a snapshottable directory")); | ||
| } |
There was a problem hiding this comment.
Can use LambdaTestUtils :
``LambdaTestUtils.intercept(SnapshotException.class,
"Directory is not a " + "snapshottable directory",
() -> hdfs.getSnapshotListing(dir1)); ``
|
@bshashikant can you please paste the command output in the PR pelase ? |
mukul1987
requested changes
Jul 27, 2020
...project/hadoop-hdfs-client/src/main/java/org/apache/hadoop/hdfs/protocol/SnapshotStatus.java
Show resolved
Hide resolved
|
Thanks @ayushtkn and @mukul1987 for the review comments. The latest patch addresses the review comments. |
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
ayushtkn
reviewed
Jul 27, 2020
Comment on lines
169
to
175
| if (rpcServer.isInvokeConcurrent(snapshotRoot)) { | ||
| Map<RemoteLocation, SnapshotStatus[]> ret = rpcClient.invokeConcurrent( | ||
| locations, remoteMethod, true, false, SnapshotStatus[].class); | ||
| return ret.values().iterator().next(); | ||
| } else { | ||
| return rpcClient.invokeSequential( | ||
| locations, remoteMethod, SnapshotStatus[].class, null); |
There was a problem hiding this comment.
Here the SnapshotStatus contains parentpath as well, the path will be the one wrt the actual namespace not with respect to mount table. You need to replace the parentpath with path corresponding to the mount entry.
if mount entry is : /mnt -> /dir in ns0, then if you trigger call on ns0 the path would be something like /dir/sub.. that you need to change to /mnt/sub..
In case of InvokeConcurrent you would be able to get Src and Dst from the ret and maybe something like this may work -
response = ret.values().iterator().next();
String src = ret.keySet().iterator().next().getSrc();
String dst = ret.keySet().iterator().next().getDest();
for (SnapshotStatus s : response) {
String mountPath =
new String(s.getParentFullPath()).replaceFirst(dst, src);
s.setParentFullPath(mountPath.getBytes());
}
For the invokeSequential one you won't be having the detail on which location did the call got success, For that you have to get it returned back from RouterRpcClient#L858, I guess to get the location returned you would require a new InvokeSequential method which returns the location as well, may be can refactor and reuse this one...
This problem would be there I think in getSnapshottableDirListing() as well. If you want, you can put a TODO and handle the location stuff in a separate follow up jira for both API's.
ayushtkn
reviewed
Jul 28, 2020
...p-hdfs-rbf/src/main/java/org/apache/hadoop/hdfs/server/federation/router/RouterSnapshot.java
Show resolved
Hide resolved
|
💔 -1 overall
This message was automatically generated. |
mukul1987
requested changes
Jul 28, 2020
...op-hdfs-rbf/src/test/java/org/apache/hadoop/hdfs/server/federation/router/TestRouterRpc.java
Outdated
Show resolved
Hide resolved
...doop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/snapshot/SnapshotManager.java
Show resolved
Hide resolved
...oop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/namenode/snapshot/TestListSnapshot.java
Outdated
Show resolved
Hide resolved
...oop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/namenode/snapshot/TestListSnapshot.java
Show resolved
Hide resolved
...oop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/namenode/snapshot/TestListSnapshot.java
Outdated
Show resolved
Hide resolved
|
Thanks for updating the patch @bshashikant. I am generally +1 for the patch. Some nit picks, minor comments. |
mukul1987
reviewed
Jul 28, 2020
|
|
||
| // Fields specific for snapshot directory | ||
| required uint32 snapshotID = 2; | ||
| required bytes parent_fullpath = 3; |
There was a problem hiding this comment.
we should also dispay if a particular snapshot is deleted/garbage collected or not.
There was a problem hiding this comment.
I think the intent was to hide the information from the user whether a snapshot is deleted or not. This is a user cmd and displaying it won't server the purpose if we are supposed to hide it.
|
💔 -1 overall
This message was automatically generated. |
mukul1987
approved these changes
Jul 29, 2020
There was a problem hiding this comment.
Thanks for updating the patch @bshashikant. +1 the patch looks good to me.
|
Give a check to test failures. |
|
💔 -1 overall
This message was automatically generated. |
Addressed in the latest patch. |
|
💔 -1 overall
This message was automatically generated. |
ayushtkn
approved these changes
Jul 29, 2020
There was a problem hiding this comment.
Thanx @bshashikant for the update,
Apart from checkstyle issues, Changes LGTM +1
|
Committed this one leaving few of the checkstyke issues ignored as similar issues still exist in other test classes. |
|
Thanks @ayushtkn and @mukul1987 for the review. |
|
💔 -1 overall
This message was automatically generated. |
Xushaohong
pushed a commit
to Xushaohong/hadoop
that referenced
this pull request
Jul 20, 2022
…hottable root with snapshot Ids. (apache#2166)
jojochuang
pushed a commit
to jojochuang/hadoop
that referenced
this pull request
May 23, 2023
…ot with snapshot Ids. (apache#2166) Conflicts: hadoop-hdfs-project/hadoop-hdfs-rbf/src/main/java/org/apache/hadoop/hdfs/server/federation/router/RouterClientProtocol.java hadoop-hdfs-project/hadoop-hdfs-rbf/src/main/java/org/apache/hadoop/hdfs/server/federation/router/RouterSnapshot.java hadoop-hdfs-project/hadoop-hdfs-rbf/src/test/java/org/apache/hadoop/hdfs/server/federation/router/TestRouterRpc.java hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/FSNamesystem.java hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/snapshot/SnapshotManager.java Change-Id: I0f61f0cede6e0570e821f318dbc47f86d18efcf7 (cherry picked from commit e9cdd09)
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.
please see https://issues.apache.org/jira/browse/HDFS-15488
COMMAND OUTPUT
sbanerjee-MBP15:hadoop-3.4.0-SNAPSHOT sbanerjee$ bin/hdfs lsSnapshottableDir
drwxr-xr-x 0 sbanerjee supergroup 0 2020-07-27 11:52 2 65536 /user
sbanerjee-MBP15:hadoop-3.4.0-SNAPSHOT sbanerjee$ bin/hdfs lsSnapshot /user
drwxr-xr-x 0 sbanerjee supergroup 0 2020-07-27 11:52 1 /user/.snapshot/s1
drwxr-xr-x 0 sbanerjee supergroup 0 2020-07-27 11:51 0 /user/.snapshot/s20200727-115156.407