HDDS-8968. [Snapshot] Distcp using snapshots fails due to incorrect toSnapshot name. #5040
Conversation
sadanand48
changed the title
ayushtkn
left a comment
ayushtkn
left a comment
There was a problem hiding this comment.
I doubt around the existing code itself, why this check and takeTemporarySnapshot stuff is there only for toSnapshot, but not for fromSnapshot?
In HDFS
getSnapshotDiffReport(dir, "", "s1");
is a valid case AFAIK, Distcp also has rdiff which would create issue in similar situation if fromSnapshot stuff isn't handled.
distcp has specific checks as well for fromSnapshot to be current dir
https://github.com/apache/hadoop/blob/trunk/hadoop-tools/hadoop-distcp/src/main/java/org/apache/hadoop/tools/DistCpSync.java#L132-L139
Thanks @ayushtkn for the review , updated the patch. |
ayushtkn
left a comment
ayushtkn
left a comment
There was a problem hiding this comment.
Minor stuff, rest looks good
...one/ozonefs-common/src/main/java/org/apache/hadoop/fs/ozone/BasicOzoneClientAdapterImpl.java
Outdated
Show resolved
Hide resolved
...one/ozonefs-common/src/main/java/org/apache/hadoop/fs/ozone/BasicOzoneClientAdapterImpl.java
Outdated
Show resolved
Hide resolved
...onefs-common/src/main/java/org/apache/hadoop/fs/ozone/BasicRootedOzoneClientAdapterImpl.java
Outdated
Show resolved
Hide resolved
...onefs-common/src/main/java/org/apache/hadoop/fs/ozone/BasicRootedOzoneClientAdapterImpl.java
Outdated
Show resolved
Hide resolved
...one/integration-test/src/test/java/org/apache/hadoop/fs/ozone/TestRootedOzoneFileSystem.java
Show resolved
Hide resolved
sumitagrawl
left a comment
sumitagrawl
left a comment
There was a problem hiding this comment.
@sadanand48 Given few minor comments, please check
...one/ozonefs-common/src/main/java/org/apache/hadoop/fs/ozone/BasicOzoneClientAdapterImpl.java
Show resolved
Hide resolved
...onefs-common/src/main/java/org/apache/hadoop/fs/ozone/BasicRootedOzoneClientAdapterImpl.java
Outdated
Show resolved
Hide resolved
| // & the toSnapshot | ||
| takeTemporaryFromSnapshot = true; | ||
| fromSnapshot = createSnapshot(snapshotDir.toString(), | ||
| "temp" + "-from-" + SnapshotInfo.generateName(Time.now())); |
There was a problem hiding this comment.
if multiple snapshot diff running, can this still be unique?
There was a problem hiding this comment.
updated code using uuid now.
ayushtkn
left a comment
ayushtkn
left a comment
There was a problem hiding this comment.
I don't have anything further. So, once the build & others are happy, it is good from my side.
Changes LGTM
| OzoneFSUtils.generateUniqueTempSnapshotName()); | ||
| } | ||
| if (fromSnapshot.isEmpty()) { | ||
| // empty fromSnapshot implies diff b/w the current state |
There was a problem hiding this comment.
@sadanand48 we don't support snapshot diff b/w snapshots where fromSnapshot is more recent than toSnapshot? Is there a use case for this?
4 participants
What changes were proposed in this pull request?
Distcp using snapshots fails due to incorrect toSnapshot name. The string used to represent current FS state should be an empty string instead of "."
Check https://github.com/apache/hadoop/blob/4e699f0383590d6c72cb3ee2294da29a4945922f/hadoop-tools/hadoop-distcp/src/main/java/org/apache/hadoop/tools/DistCpSync.java#L373 for reference.
What is the link to the Apache JIRA
https://issues.apache.org/jira/browse/HDDS-8968
How was this patch tested?
Tested manually