Copilot commented on code in PR #9162:
URL: https://github.com/apache/ozone/pull/9162#discussion_r2436994311
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/snapshot/SnapshotUtils.java:
##########
@@ -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.
Review 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.'
```suggestion
/**
* 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.
*/
```
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/key/OMKeyPurgeRequest.java:
##########
@@ -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);
}
Review 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.
```suggestion
}
} 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));
```
--
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]
