Copilot commented on code in PR #10901:
URL: https://github.com/apache/ozone/pull/10901#discussion_r3677200839
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/protocolPB/OzoneManagerRequestHandler.java:
##########
@@ -1482,6 +1482,10 @@ private SnapshotDiffResponse snapshotDiff(
builder.setSnapshotDiffReport(
response.getSnapshotDiffReport().toProtobuf());
}
+ if (response.getSubStatus() != null) {
+ builder.setSubStatus(response.getSubStatus().toProtoBuf());
+ builder.setProgressPercent(response.getProgressPercent());
+ }
Review Comment:
progressPercent is currently serialized whenever subStatus is set, even for
sub-statuses that don’t support progress (e.g., PATH_RESOLUTION_FSO /
DIFF_REPORT_GEN). To keep API semantics clear (“when available”), only set
progressPercent when subStatus.hasProgress().
##########
hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/om/protocolPB/OzoneManagerProtocolClientSideTranslatorPB.java:
##########
@@ -1468,12 +1468,17 @@ private SnapshotDiffResponse
snapshotDiffInternal(String volumeName,
OzoneManagerProtocolProtos.SnapshotDiffResponse diffResponse =
omResponse.getSnapshotDiffResponse();
- return new SnapshotDiffResponse(SnapshotDiffReportOzone.fromProtobuf(
- diffResponse.getSnapshotDiffReport()),
+ SnapshotDiffResponse result = new SnapshotDiffResponse(
+
SnapshotDiffReportOzone.fromProtobuf(diffResponse.getSnapshotDiffReport()),
JobStatus.fromProtobuf(diffResponse.getJobStatus()),
diffResponse.getWaitTimeInMs(),
diffResponse.getReason(),
reportOnly);
+ if (diffResponse.hasSubStatus()) {
+
result.setSubStatus(SnapshotDiffResponse.SubStatus.fromProtoBuf(diffResponse.getSubStatus()));
+ result.setProgressPercent(diffResponse.getProgressPercent());
+ }
Review Comment:
Client-side translator reads diffResponse.getProgressPercent() without
checking hasProgressPercent(). If the server omits progressPercent (or an older
server is contacted), this loses the “unset vs 0.0” distinction and can mislead
consumers. Guard the read with hasProgressPercent().
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/snapshot/SnapshotDiffManager.java:
##########
@@ -1244,10 +1257,14 @@ void addToObjectIdMap(Table<String, ? extends
WithParentObjectId> fsTable,
PersistentMap<byte[], Boolean> objectIdToIsDirMap,
Optional<Set<Long>> oldParentIds,
Optional<Set<Long>> newParentIds,
- TablePrefixInfo tablePrefixes, String jobKey) throws IOException,
RocksDBException {
+ TablePrefixInfo tablePrefixes, String jobKey,
+ String jobId) throws IOException, RocksDBException {
if (deltaFiles.isEmpty()) {
return;
}
+ long objectIdMapStart = Time.monotonicNow();
+ updateProgress(jobKey, 0.0);
+ AtomicLong keysProcessed = new AtomicLong(0);
Review Comment:
Progress reset is skipped when deltaFiles is empty because
updateProgress(jobKey, 0.0) happens after the early return. This can leave a
previous stage’s progressPercent visible for subsequent stages, which
contradicts the intended “reset at start” behavior.
--
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]
