Copilot commented on code in PR #9985:
URL: https://github.com/apache/ozone/pull/9985#discussion_r3090558435
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/snapshot/SnapshotDiffManager.java:
##########
@@ -556,6 +560,129 @@ public SnapshotDiffResponse getSnapshotDiffReport(
}
}
+ public SnapshotDiffResponse getSnapshotDiffReport(
+ final String volumeName,
+ final String bucketName,
+ final String fromSnapshotName,
+ final String toSnapshotName,
+ final String index,
+ final int pageSize
+ ) throws IOException {
+
+ SnapshotInfo fsInfo = getSnapshotInfo(ozoneManager,
+ volumeName, bucketName, fromSnapshotName);
+ SnapshotInfo tsInfo = getSnapshotInfo(ozoneManager,
+ volumeName, bucketName, toSnapshotName);
+
+ String snapDiffJobKey = generateSnapDiffJobKey.apply(fsInfo, tsInfo);
+ SnapshotDiffJob snapDiffJob = snapDiffJobTable.get(snapDiffJobKey);
+ OFSPath snapshotRoot = getSnapshotRootPath(volumeName, bucketName);
+
+ if (snapDiffJob == null) {
+ return new SnapshotDiffResponse(
+ new SnapshotDiffReportOzone(snapshotRoot.toString(), volumeName,
bucketName,
+ fromSnapshotName, toSnapshotName,
+ new ArrayList<>(), null), NOT_FOUND, defaultWaitTime);
Review Comment:
In the report-only getSnapshotDiffReport(...) path, the NOT_FOUND case
builds SnapshotDiffResponse without setting isReportOnly=true. This will make
SnapshotDiffResponse#toString() emit the submit-call error text ("Invalid state
for submit snapshot diff call") instead of the intended "No snapshot diff job
found..." guidance. Construct the response with the isReportOnly constructor
(or otherwise set the flag) for NOT_FOUND in this method.
```suggestion
new ArrayList<>(), null), NOT_FOUND, defaultWaitTime, true);
```
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/protocolPB/OzoneManagerRequestHandler.java:
##########
@@ -1387,16 +1394,28 @@ private GetS3VolumeContextResponse getS3VolumeContext()
@DisallowedUntilLayoutVersion(FILESYSTEM_SNAPSHOT)
private SnapshotDiffResponse snapshotDiff(
SnapshotDiffRequest snapshotDiffRequest) throws IOException {
- org.apache.hadoop.ozone.snapshot.SnapshotDiffResponse response =
- impl.snapshotDiff(
- snapshotDiffRequest.getVolumeName(),
- snapshotDiffRequest.getBucketName(),
- snapshotDiffRequest.getFromSnapshot(),
- snapshotDiffRequest.getToSnapshot(),
- snapshotDiffRequest.getToken(),
- snapshotDiffRequest.getPageSize(),
- snapshotDiffRequest.getForceFullDiff(),
- snapshotDiffRequest.getDisableNativeDiff());
+ org.apache.hadoop.ozone.snapshot.SnapshotDiffResponse response;
+
+ if (snapshotDiffRequest.hasForceFullDiff() &&
snapshotDiffRequest.hasDisableNativeDiff()) {
+ response = impl.snapshotDiff(
+ snapshotDiffRequest.getVolumeName(),
+ snapshotDiffRequest.getBucketName(),
+ snapshotDiffRequest.getFromSnapshot(),
+ snapshotDiffRequest.getToSnapshot(),
+ snapshotDiffRequest.getToken(),
+ snapshotDiffRequest.getPageSize(),
+ snapshotDiffRequest.getForceFullDiff(),
+ snapshotDiffRequest.getDisableNativeDiff());
+ } else {
+ response = impl.snapshotDiff(
+ snapshotDiffRequest.getVolumeName(),
+ snapshotDiffRequest.getBucketName(),
+ snapshotDiffRequest.getFromSnapshot(),
+ snapshotDiffRequest.getToSnapshot(),
+ snapshotDiffRequest.getToken(),
+ snapshotDiffRequest.getPageSize());
+ }
Review Comment:
snapshotDiff() routing relies on both hasForceFullDiff() &&
hasDisableNativeDiff() to detect the legacy (submit+report) request. If a
client sets only one of these (they are optional proto2 fields), the handler
will route to the report-only overload and ignore the provided flag, which
changes behavior. Consider treating the request as legacy if either deprecated
flag is present (hasForceFullDiff() || hasDisableNativeDiff()), and default the
other flag as needed.
##########
hadoop-ozone/cli-shell/src/main/java/org/apache/hadoop/ozone/shell/snapshot/SnapshotDiffHandler.java:
##########
@@ -109,15 +117,43 @@ protected void execute(OzoneClient client, OzoneAddress
address)
if (cancel) {
cancelSnapshotDiff(client.getObjectStore(), volumeName, bucketName);
- } else {
+ } else if (getReport) {
getSnapshotDiff(client.getObjectStore(), volumeName, bucketName);
+ } else {
+ submitSnapshotDiff(client.getObjectStore(), volumeName, bucketName);
+ }
+ }
+
+ private void submitSnapshotDiff(ObjectStore store, String volumeName,
+ String bucketName) throws IOException {
+ SubmitSnapshotDiffResponse response;
+ try {
+ response = store.submitSnapshotDiff(volumeName, bucketName,
fromSnapshot, toSnapshot,
+ forceFullDiff, diffDisableNativeLibs);
+ } catch (OMException ex) {
+ // submit snapshot diff falls back to legacy snapshotDiff is server does
not support it.
+ if (ex.getResult() == OMException.ResultCodes.NOT_SUPPORTED_OPERATION) {
+ SnapshotDiffResponse diffResponse = store.snapshotDiff(volumeName,
bucketName, fromSnapshot, toSnapshot,
+ token, pageSize, forceFullDiff, diffDisableNativeLibs);
+ try (PrintWriter writer = out()) {
+ if (json) {
+
writer.println(toJsonStringWithDefaultPrettyPrinter(getJsonObject(diffResponse)));
+ } else {
+ writer.println(diffResponse);
+ }
+ }
+ }
+ return;
Review Comment:
submitSnapshotDiff() catches OMException but returns silently for all result
codes other than NOT_SUPPORTED_OPERATION. This will hide real submission
failures from the CLI user (no output and exit code likely 0). Re-throw the
exception (or print an error and propagate a non-zero exit) when the
OMException is not the specific fallback case.
```suggestion
return;
}
throw ex;
```
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/snapshot/SnapshotDiffManager.java:
##########
@@ -556,6 +560,129 @@ public SnapshotDiffResponse getSnapshotDiffReport(
}
}
+ public SnapshotDiffResponse getSnapshotDiffReport(
+ final String volumeName,
+ final String bucketName,
+ final String fromSnapshotName,
+ final String toSnapshotName,
+ final String index,
+ final int pageSize
+ ) throws IOException {
+
+ SnapshotInfo fsInfo = getSnapshotInfo(ozoneManager,
+ volumeName, bucketName, fromSnapshotName);
+ SnapshotInfo tsInfo = getSnapshotInfo(ozoneManager,
+ volumeName, bucketName, toSnapshotName);
+
+ String snapDiffJobKey = generateSnapDiffJobKey.apply(fsInfo, tsInfo);
+ SnapshotDiffJob snapDiffJob = snapDiffJobTable.get(snapDiffJobKey);
+ OFSPath snapshotRoot = getSnapshotRootPath(volumeName, bucketName);
+
+ if (snapDiffJob == null) {
+ return new SnapshotDiffResponse(
+ new SnapshotDiffReportOzone(snapshotRoot.toString(), volumeName,
bucketName,
+ fromSnapshotName, toSnapshotName,
+ new ArrayList<>(), null), NOT_FOUND, defaultWaitTime);
+ }
+
+ switch (snapDiffJob.getStatus()) {
+ case QUEUED:
+ case REJECTED:
+ return new SnapshotDiffResponse(
+ new SnapshotDiffReportOzone(snapshotRoot.toString(), volumeName,
+ bucketName, fromSnapshotName, toSnapshotName, new ArrayList<>(),
+ null),
+ snapDiffJob.getStatus(), defaultWaitTime, true);
+ case IN_PROGRESS:
+ SnapshotDiffResponse response = new SnapshotDiffResponse(
+ new SnapshotDiffReportOzone(snapshotRoot.toString(), volumeName,
bucketName,
+ fromSnapshotName, toSnapshotName,
+ new ArrayList<>(), null), IN_PROGRESS, defaultWaitTime, true);
+ response.setSubStatus(snapDiffJob.getSubStatus());
+ response.setProgressPercent(snapDiffJob.getKeysProcessedPct());
+ return response;
+ case FAILED:
+ return new SnapshotDiffResponse(
+ new SnapshotDiffReportOzone(snapshotRoot.toString(), volumeName,
+ bucketName, fromSnapshotName, toSnapshotName, new ArrayList<>(),
+ null), FAILED,
+ // waitTime is equal to clean up internal. After that job will be
+ // removed and client can retry.
+ ozoneManager.getOmSnapshotManager().getDiffCleanupServiceInterval(),
+ snapDiffJob.getReason(), true);
+ case DONE:
+ SnapshotDiffReportOzone report = createPageResponse(snapDiffJob,
+ volumeName, bucketName, fromSnapshotName, toSnapshotName, index,
+ pageSize);
+ return new SnapshotDiffResponse(report, DONE, 0L, true);
+ case CANCELLED:
+ return new SnapshotDiffResponse(
+ new SnapshotDiffReportOzone(snapshotRoot.toString(), volumeName,
+ bucketName, fromSnapshotName, toSnapshotName, new ArrayList<>(),
+ null),
+ CANCELLED, 0L, true);
+ default:
+ throw new IllegalStateException("Unknown snapshot job status: " +
+ snapDiffJob.getStatus());
+ }
+ }
+
+ public synchronized SubmitSnapshotDiffResponse submitSnapshotDiff(
+ final String volumeName,
+ final String bucketName,
+ final String fromSnapshotName,
+ final String toSnapshotName,
+ final boolean forceFullDiff,
+ final boolean disableNativeDiff
+ ) throws IOException {
+ SnapshotInfo fsInfo = getSnapshotInfo(ozoneManager,
+ volumeName, bucketName, fromSnapshotName);
+ SnapshotInfo tsInfo = getSnapshotInfo(ozoneManager,
+ volumeName, bucketName, toSnapshotName);
+
+ String snapDiffJobKey = generateSnapDiffJobKey.apply(fsInfo, tsInfo);
+
+ SnapshotDiffJob snapDiffJob = getSnapDiffReportStatus(snapDiffJobKey,
+ volumeName, bucketName, fromSnapshotName, toSnapshotName,
+ forceFullDiff, disableNativeDiff);
+
+ JobStatus prevStatus = snapDiffJob.getStatus();
+ String prevReason = isEmpty(snapDiffJob.getReason()) ? null :
snapDiffJob.getReason();
+ String jobId = prevStatus == QUEUED ? snapDiffJob.getJobId() :
UUID.randomUUID().toString();
+
+ switch (prevStatus) {
+ case QUEUED:
+ case FAILED:
+ case REJECTED:
+ case CANCELLED:
+ LOG.info("Submitting snapshot diff generation request for" +
+ " volume: {}, bucket: {}, fromSnapshot: {} and toSnapshot: {}",
+ volumeName, bucketName, fromSnapshotName, toSnapshotName);
+ try {
+ updateJobStatus(snapDiffJobKey, prevStatus, IN_PROGRESS);
+ snapDiffExecutor.execute(() ->
generateSnapshotDiffReport(snapDiffJobKey, jobId,
+ volumeName, bucketName, fromSnapshotName, toSnapshotName,
+ forceFullDiff, disableNativeDiff));
+ return new SubmitSnapshotDiffResponse(defaultWaitTime, prevStatus,
prevReason);
Review Comment:
submitSnapshotDiff() generates a new jobId for resubmits
(FAILED/REJECTED/CANCELLED) but does not update the persisted SnapshotDiffJob's
jobId in snapDiffJobTable. Since report retrieval uses snapDiffJob.getJobId()
as the report-key prefix, the newly generated report entries will not be
readable (and integrity checks can mark the job FAILED). When resubmitting,
update the job record with the new jobId (and consider clearing/resetting any
previous report metadata like largestEntryKey/totalDiffEntries/reason/progress).
--
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]
