[GitHub] [hadoop] szetszwo commented on pull request #5532: HDFS-16972. Delete a snapshot may deleteCurrentFile.
szetszwo commented on PR #5532: URL: https://github.com/apache/hadoop/pull/5532#issuecomment-1525780573 @umamaheswararao , thanks for reviewing this! > Could you also check test failures? The test failures are related. Thanks for filing [HDFS-16996](https://issues.apache.org/jira/browse/HDFS-16996). Unfortunately, `ExitUtil` did not print out that stack trace of the `ClassCastException`. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: common-issues-unsubscr...@hadoop.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: common-issues-unsubscr...@hadoop.apache.org For additional commands, e-mail: common-issues-h...@hadoop.apache.org
[GitHub] [hadoop] szetszwo commented on pull request #5532: HDFS-16972. Delete a snapshot may deleteCurrentFile.
szetszwo commented on PR #5532:
URL: https://github.com/apache/hadoop/pull/5532#issuecomment-1516773369
> ... then there should be separate JIRA for it? ...
In the past few days, I was testing whether we need the change below. It
turned that the change lead to a problem that snapshot diffs from rename src
and dst paths can mix together. So the change is incorrect. The current code
seems correct.
```java
-snapshotId = lastSnapshot;
+if (lastSnapshot > snapshotId) {
+ snapshotId = lastSnapshot;
+}
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: common-issues-unsubscr...@hadoop.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: common-issues-unsubscr...@hadoop.apache.org
For additional commands, e-mail: common-issues-h...@hadoop.apache.org
[GitHub] [hadoop] szetszwo commented on pull request #5532: HDFS-16972. Delete a snapshot may deleteCurrentFile.
szetszwo commented on PR #5532:
URL: https://github.com/apache/hadoop/pull/5532#issuecomment-1511669706
> Are we planning to address here? [#5532
(comment)](https://github.com/apache/hadoop/pull/5532#issuecomment-1499923239)
Probably not. I found a potential fix shown below -- it should update
`snapshotId` only if it is larger. It might be able to fix the bug. However,
it is a very big change since it change the rename diff entries and completely
change the rename-cleanup-algorithm.
```java
+++
b/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/INodesInPath.java
@@ -181,7 +181,9 @@ static INodesInPath resolve(final INodeDirectory
startingDir,
(sf =
curNode.asDirectory().getDirectoryWithSnapshotFeature()) != null) {
lastSnapshot = sf.getLastSnapshotId();
}
-snapshotId = lastSnapshot;
+if (lastSnapshot > snapshotId) {
+ snapshotId = lastSnapshot;
+}
}
}
}
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: common-issues-unsubscr...@hadoop.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: common-issues-unsubscr...@hadoop.apache.org
For additional commands, e-mail: common-issues-h...@hadoop.apache.org
[GitHub] [hadoop] szetszwo commented on pull request #5532: HDFS-16972. Delete a snapshot may deleteCurrentFile.
szetszwo commented on PR #5532: URL: https://github.com/apache/hadoop/pull/5532#issuecomment-1499923239 > ... when call reaches here, the snapshot id is CURRENT_STATE_ID, which represent current state. Why are we proceeding to clean subtree originally with CURRENT_STATE_ID. Could you provide more details on that? ... The might be a bug. `prior` and `snapshot` are supposed to be the prior and the current Snapshot IDs. Because of rename, the subtree was moved from somewhere to there. The logic in `getPriorSnapshot` and `getSelfSnapshot` may be incorrect. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: common-issues-unsubscr...@hadoop.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: common-issues-unsubscr...@hadoop.apache.org For additional commands, e-mail: common-issues-h...@hadoop.apache.org
