hemantk-12 commented on code in PR #4819:
URL: https://github.com/apache/ozone/pull/4819#discussion_r1222287832
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/snapshot/SnapshotDiffJob.java:
##########
@@ -61,6 +62,7 @@ public SnapshotDiffJob(long creationTime,
String fromSnapshot,
String toSnapshot,
boolean forceFullDiff,
+ boolean cancel,
Review Comment:
Isn't `jobStatus` enough to tell if job is cancelled or not?
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/snapshot/SnapshotDiffManager.java:
##########
@@ -389,9 +399,19 @@ public SnapshotDiffResponse getSnapshotDiffReport(
String snapDiffJobKey = fsInfo.getSnapshotID() + DELIMITER +
tsInfo.getSnapshotID();
+ // Check if the job already exists in the table
+ // before getting report status.
+ if (cancel && Objects.nonNull(snapDiffJobTable.get(snapDiffJobKey)) &&
Review Comment:
There are few problems by adding this to current creation flow.
1. If job doesn't exist, we would create a new job. Which is not needed.
Correct behavior should be return empty response with a message.
1. If job is not `IN_PROGRESS` status and it is `DONE`, it will not mark the
job cancel and will return first page response. I am not sure if that is
correct response to client.
1. It will fail at line 409, if two or more commands are issued at the same
time for the same snapDiff job because one of them will mark it cancel
successfully and other/s will fail due to
[check](https://github.com/apache/ozone/blob/9034434285f3877246b3ebc6a7513c082931b279/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/snapshot/SnapshotDiffManager.java#L1164).
I think it would be better to create a separate flow for cancel and handle
these cases appropriately. Also snapDiff job submission and cancellation should
be consider two separate options in `OmSnapshotManager` and
`SnapshotDiffManager`.
In
[OzoneManager](https://github.com/apache/ozone/pull/4819/files#diff-54bbfc5b5dd89a4d035bee206c53d3d11bbb579a2957abca589e313283d98af9R4523),
you can use cancel to decide which flow needs to be called.
```
public SnapshotDiffResponse snapshotDiff(String volume,
String bucket,
String fromSnapshot,
String toSnapshot,
String token,
int pageSize,
boolean forceFullDiff,
boolean cancel)
throws IOException {
if (cancel) {
return omSnapshotManager.cancelSnapshotDiff(volume, bucket,
fromSnapshot, toSnapshot);
} else {
return omSnapshotManager.getSnapshotDiffReport(volume, bucket,
fromSnapshot, toSnapshot, token, pageSize, forceFullDiff);
}
}
```
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/snapshot/SnapshotDiffManager.java:
##########
@@ -422,6 +442,12 @@ public SnapshotDiffResponse getSnapshotDiffReport(
bucketName, fromSnapshotName, toSnapshotName, new ArrayList<>(),
null),
REJECTED, defaultWaitTime);
+ case CANCELED:
+ return new SnapshotDiffResponse(
+ new SnapshotDiffReportOzone(snapshotRoot.toString(), volumeName,
+ bucketName, fromSnapshotName, toSnapshotName, new ArrayList<>(),
+ null),
+ CANCELED, defaultWaitTime);
Review Comment:
Return zero instead of `defaultWaitTime` here.
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/snapshot/SnapshotDiffManager.java:
##########
@@ -718,36 +746,60 @@ private void generateSnapshotDiffReport(final String
jobKey,
Table<String, OmKeyInfo> tsKeyTable = toSnapshot.getMetadataManager()
.getKeyTable(bucketLayout);
- getDeltaFilesAndDiffKeysToObjectIdToKeyMap(fsKeyTable, tsKeyTable,
- fromSnapshot, toSnapshot, fsInfo, tsInfo, useFullDiff,
- tablePrefixes, objectIdToKeyNameMapForFromSnapshot,
- objectIdToKeyNameMapForToSnapshot, objectIDsToCheckMap,
- path.toString());
-
- if (bucketLayout.isFileSystemOptimized()) {
- validateSnapshotsAreActive(volumeName, bucketName, fromSnapshotName,
- toSnapshotName);
-
- Table<String, OmDirectoryInfo> fsDirTable =
- fromSnapshot.getMetadataManager().getDirectoryTable();
- Table<String, OmDirectoryInfo> tsDirTable =
- toSnapshot.getMetadataManager().getDirectoryTable();
-
- getDeltaFilesAndDiffKeysToObjectIdToKeyMap(fsDirTable, tsDirTable,
- fromSnapshot, toSnapshot, fsInfo, tsInfo, useFullDiff,
- tablePrefixes, objectIdToKeyNameMapForFromSnapshot,
- objectIdToKeyNameMapForToSnapshot, objectIDsToCheckMap,
- path.toString());
- }
+ // These are the most time and resource consuming method calls.
+ // Split the calls into steps and store them in an array, to avoid
+ // repetition while constantly checking if the job is cancelled.
+ Callable<Void>[] methodCalls = new Callable[]{
+ () -> {
+ getDeltaFilesAndDiffKeysToObjectIdToKeyMap(fsKeyTable, tsKeyTable,
+ fromSnapshot, toSnapshot, fsInfo, tsInfo, useFullDiff,
+ tablePrefixes, objectIdToKeyNameMapForFromSnapshot,
+ objectIdToKeyNameMapForToSnapshot, objectIDsToCheckMap,
+ path.toString());
+ return null;
+ },
+ () -> {
+ if (bucketLayout.isFileSystemOptimized()) {
+ Table<String, OmDirectoryInfo> fsDirTable =
+ fromSnapshot.getMetadataManager().getDirectoryTable();
+ Table<String, OmDirectoryInfo> tsDirTable =
+ toSnapshot.getMetadataManager().getDirectoryTable();
+
+ getDeltaFilesAndDiffKeysToObjectIdToKeyMap(fsDirTable,
tsDirTable,
+ fromSnapshot, toSnapshot, fsInfo, tsInfo, useFullDiff,
+ tablePrefixes, objectIdToKeyNameMapForFromSnapshot,
+ objectIdToKeyNameMapForToSnapshot, objectIDsToCheckMap,
+ path.toString());
+ }
+ return null;
+ },
+ () -> {
+ long totalDiffEntries = generateDiffReport(jobKey,
+ jobId,
+ objectIDsToCheckMap,
+ objectIdToKeyNameMapForFromSnapshot,
+ objectIdToKeyNameMapForToSnapshot);
+ // If job is canceled, totalDiffEntries will be equal to -1.
+ if (totalDiffEntries >= 0 &&
+ !snapDiffJobTable.get(jobKey)
+ .getStatus().equals(CANCELED)) {
+ updateJobStatusToDone(jobKey, totalDiffEntries);
+ }
+ return null;
+ }
+ };
- validateSnapshotsAreActive(volumeName, bucketName, fromSnapshotName,
- toSnapshotName);
- long totalDiffEntries = generateDiffReport(jobId,
- objectIDsToCheckMap,
- objectIdToKeyNameMapForFromSnapshot,
- objectIdToKeyNameMapForToSnapshot);
+ // Check if the job is cancelled, before every method call.
+ for (Callable<Void> methodCall : methodCalls) {
+ if (snapDiffJobTable.get(jobKey).getStatus()
+ .equals(CANCELED)) {
+ return;
+ }
Review Comment:
Can you please move this to another function
`validateSnapshotDiffJobIsActive`? An reuse it at other place like lines
1071-1073 and lines 784-786.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
