HDDS-13799. Revert "HDDS-12982. Reduce log level for snapshot validation failure (#8851)" #9162
Conversation
…ion failure (apache#8851)" This reverts commit ba0ecb2. Change-Id: Ia349cd43ef70ca6d58500fd5509a7707217ca2ad
There was a problem hiding this comment.
Pull Request Overview
This PR reverts the prior change that downgraded snapshot validation failures to warnings, restoring exception-based validation flow to prevent silent acceptance of invalid snapshot chains.
- Change validatePreviousSnapshotId to throw OMException on mismatch instead of returning boolean
- Update OMKeyPurgeRequest to rely on exception flow rather than boolean return
- Add explanatory comments around validation behavior
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/snapshot/SnapshotUtils.java | Converts boolean-returning validation to exception-throwing, adds method-level comments. |
| hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/key/OMKeyPurgeRequest.java | Updates call site to the new void/exception validation method and removes inline error response branch. |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| // 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. |
There was a problem hiding this comment.
[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.'
| // 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. | |
| */ |
| } | ||
| validatePreviousSnapshotId(fromSnapshotInfo, omMetadataManager.getSnapshotChainManager(), | ||
| expectedPreviousSnapshotId); | ||
| } |
There was a problem hiding this comment.
[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.
| } | |
| } | |
| } 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)); |
2 participants
What changes were proposed in this pull request?
HDDS-13799. Revert "HDDS-12982. Reduce log level for snapshot validation failure (#8851)"
Please describe your PR in detail:
What is the link to the Apache JIRA
https://issues.apache.org/jira/browse/HDDS-13799
How was this patch tested?