Copilot commented on code in PR #11808:
URL: https://github.com/apache/ozone/pull/11808#discussion_r4395677387
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/snapshot/SnapshotDiffManager.java:
##########
@@ -798,14 +801,23 @@ Pair<List<DiffReportEntry>, String> createPageResponse(
* service and throws the exception to client.
*/
@VisibleForTesting
- void checkReportsIntegrity(final SnapshotDiffJob diffJob,
+ synchronized void checkReportsIntegrity(final SnapshotDiffJob diffJob,
+ final String jobKey,
final String largestPageIndex,
boolean lastPage) throws IOException {
- // For last page check last entry returned if the largest entry key equals
the largest key stored in the job entry.
- if (lastPage && (diffJob.getLargestEntryKey() == null ||
!diffJob.getLargestEntryKey().equals(largestPageIndex))) {
+ // A DONE job persisted before largestEntryKey was introduced deserializes
with a
+ // null largestEntryKey. Such a legacy job carries no stored last-entry to
compare,
+ // so skip the integrity assertion for it rather than treating it as a
failure.
+ if (lastPage && diffJob.getLargestEntryKey() != null &&
+ !diffJob.getLargestEntryKey().equals(largestPageIndex)) {
LOG.error("Expected last entry: {} but found " +
"Largest Page entry: {}", diffJob.getLargestEntryKey(),
largestPageIndex);
- updateJobStatus(diffJob.getJobId(), DONE, FAILED);
+ // The job is stored under jobKey (fromSnapshotId + DELIMITER +
toSnapshotId), not
+ // under its jobId; passing the jobId here would look up a missing row.
+ SnapshotDiffJob currentJob = snapDiffJobTable.get(jobKey);
+ if (currentJob != null &&
currentJob.getJobId().equals(diffJob.getJobId()) && currentJob.getStatus() ==
DONE) {
+ updateJobStatus(jobKey, DONE, FAILED);
Review Comment:
`SnapshotDiffCleanupService.moveOldSnapDiffJobsToPurgeTable()` deletes
expired DONE rows without taking the manager lock. If cleanup deletes this row
between this lookup and the second lookup in `updateJobStatus()`, recovery
still throws an NPE instead of the intended IOException. Coordinate cleanup
deletion and the status transition through shared synchronization or an atomic
database operation, and add a regression test that deletes the row between the
two lookups.
--
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]
