Skip to content

HDDS-13799. Revert "HDDS-12982. Reduce log level for snapshot validation failure (#8851)" #9162

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
swamirishi merged 1 commit into from
Oct 18, 2025
+9 −10
Merged

HDDS-13799. Revert "HDDS-12982. Reduce log level for snapshot validation failure (#8851)" #9162

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 2 additions & 5 deletions ...ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/key/OMKeyPurgeRequest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -97,11 +97,8 @@ public OMClientResponse validateAndUpdateCache(OzoneManager ozoneManager, Execut
// redundant tombstone entry in the deletedTable. It is better to skip the transaction.
UUID expectedPreviousSnapshotId = purgeKeysRequest.getExpectedPreviousSnapshotID().hasUuid()
? fromProtobuf(purgeKeysRequest.getExpectedPreviousSnapshotID().getUuid()) : null;
if (!validatePreviousSnapshotId(fromSnapshotInfo, omMetadataManager.getSnapshotChainManager(),
expectedPreviousSnapshotId)) {
return new OMKeyPurgeResponse(createErrorOMResponse(omResponse,
new OMException("Snapshot validation failed", OMException.ResultCodes.INVALID_REQUEST)));
}
validatePreviousSnapshotId(fromSnapshotInfo, omMetadataManager.getSnapshotChainManager(),
expectedPreviousSnapshotId);
}
Copy link

Copilot AI Oct 16, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[nitpick] Now that validatePreviousSnapshotId throws OMException (INVALID_REQUEST) for a client-side validation failure, consider catching OMException separately to avoid logging an expected client error at ERROR level and to return a typed error response. For example: catch (OMException e) { return new OMKeyPurgeResponse(createErrorOMResponse(omResponse, e)); } then have a separate catch (IOException e) for genuine IO errors.

Suggested change
}
}
} catch (OMException e) {
// Client-side validation failure, do not log at ERROR level.
if (LOG.isDebugEnabled()) {
AUDIT.logWriteFailure(ozoneManager.buildAuditMessageForFailure(OMSystemAction.KEY_DELETION, null, e));
}
return new OMKeyPurgeResponse(createErrorOMResponse(omResponse, e));

Copilot uses AI. Check for mistakes.
} catch (IOException e) {
LOG.error("Error occurred while performing OmKeyPurge. ", e);
Expand Down
12 changes: 7 additions & 5 deletions ...-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/snapshot/SnapshotUtils.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -292,17 +292,19 @@ public static UUID getLatestPathSnapshotId(String volumeName, String bucketName,
return snapshotChainManager.getLatestPathSnapshotId(snapshotPath);
}

public static boolean validatePreviousSnapshotId(SnapshotInfo snapshotInfo,
// Validates the previous path snapshotId for given a snapshotInfo. In case snapshotInfo is
// null, the snapshotInfo would be considered as AOS and previous snapshot becomes the latest snapshot in the global
// snapshot chain. Would throw OMException if validation fails otherwise function would pass.
Comment on lines +295 to +297
Copy link

Copilot AI Oct 16, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[nitpick] Please convert these line comments to a proper Javadoc for this public API, clarify grammar, and expand the undefined acronym 'AOS'. For example: describe that if snapshotInfo is null the latest global snapshot is used, document parameters, and explicitly declare @throws OMException for validation failures. Suggested phrasing: 'Validates the previous path snapshot ID for the given snapshotInfo; if snapshotInfo is null, uses the latest ID from the global chain; throws OMException (INVALID_REQUEST) if the expected ID does not match.'

Suggested change
// Validates the previous path snapshotId for given a snapshotInfo. In case snapshotInfo is
// null, the snapshotInfo would be considered as AOS and previous snapshot becomes the latest snapshot in the global
// snapshot chain. Would throw OMException if validation fails otherwise function would pass.
/**
* Validates the previous path snapshot ID for the given {@link SnapshotInfo}.
* <p>
* If {@code snapshotInfo} is {@code null}, the latest global snapshot ID from the
* snapshot chain is used for validation. This is typically the case when operating
* on the active object store (AOS), i.e., when no snapshot context is provided.
* <p>
* If the expected previous snapshot ID does not match the actual previous snapshot ID,
* this method throws an {@link OMException} with {@code INVALID_REQUEST}.
*
* @param snapshotInfo The snapshot information to validate against. If {@code null},
* the latest global snapshot is used.
* @param snapshotChainManager The manager for snapshot chains.
* @param expectedPreviousSnapshotId The expected previous snapshot ID to validate.
* @throws OMException if the expected previous snapshot ID does not match the actual previous snapshot ID.
* @throws IOException if an I/O error occurs during validation.
*/

Copilot uses AI. Check for mistakes.
public static void validatePreviousSnapshotId(SnapshotInfo snapshotInfo,
SnapshotChainManager snapshotChainManager,
UUID expectedPreviousSnapshotId) throws IOException {
UUID previousSnapshotId = snapshotInfo == null ? snapshotChainManager.getLatestGlobalSnapshotId() :
SnapshotUtils.getPreviousSnapshotId(snapshotInfo, snapshotChainManager);
if (!Objects.equals(expectedPreviousSnapshotId, previousSnapshotId)) {
LOG.warn("Snapshot validation failed. Expected previous snapshotId : " +
expectedPreviousSnapshotId + " but was " + previousSnapshotId);
return false;
throw new OMException("Snapshot validation failed. Expected previous snapshotId : " +
expectedPreviousSnapshotId + " but was " + previousSnapshotId,
OMException.ResultCodes.INVALID_REQUEST);
}
return true;
}

/**
Expand Down