chungen0126 commented on code in PR #10183:
URL: https://github.com/apache/ozone/pull/10183#discussion_r3183819413
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/key/OMKeyRequest.java:
##########
@@ -1308,46 +1308,153 @@ protected void validateEncryptionKeyInfo(OmBucketInfo
bucketInfo, KeyArgs keyArg
}
/**
- * Validates atomic rewrite conditions for conditional writes.
- * <p>
- * For If-None-Match: * (expectedDataGeneration =
EXPECTED_GEN_CREATE_IF_NOT_EXISTS),
- * the key must NOT exist.
- * <p>
- * For If-Match with a specific generation, the key must exist with matching
updateID.
+ * Resolves conditional write state at request admission time.
*
- * @param dbKeyInfo the existing key info from the database (null if key
doesn't exist)
- * @param keyArgs the key arguments containing expected generation
- * @throws OMException if validation fails
+ * This validates expected generations as admission-time preconditions and
+ * rewrites matching ETags into expected generations for commit-time checks.
*/
- protected void validateAtomicRewrite(OmKeyInfo dbKeyInfo, KeyArgs keyArgs)
- throws OMException {
+ protected KeyArgs resolveConditionalWriteAtAdmission(OmKeyInfo dbKeyInfo,
+ KeyArgs keyArgs, Map<String, String> auditMap) throws OMException {
if (keyArgs.hasExpectedDataGeneration()) {
- long expectedGen = keyArgs.getExpectedDataGeneration();
- // If expectedGen is EXPECTED_GEN_CREATE_IF_NOT_EXISTS, it means the key
MUST NOT exist (If-None-Match)
- if (expectedGen == OzoneConsts.EXPECTED_GEN_CREATE_IF_NOT_EXISTS) {
- if (dbKeyInfo != null) {
- throw new OMException("Key already exists",
- OMException.ResultCodes.KEY_ALREADY_EXISTS);
- }
- } else {
- // If a key does not exist, or if it exists but the updateID do not
match, then fail this request.
- if (dbKeyInfo == null) {
- throw new OMException("Key not found during expected rewrite",
- OMException.ResultCodes.KEY_NOT_FOUND);
- }
- if (dbKeyInfo.getUpdateID() != expectedGen) {
- throw new OMException("Generation mismatch during expected rewrite",
- OMException.ResultCodes.KEY_NOT_FOUND);
- }
+ validateAtomicRewriteAtAdmission(dbKeyInfo, keyArgs, auditMap);
+ return keyArgs;
+ }
+
+ KeyArgs resolvedKeyArgs = validateAndRewriteIfMatchAsExpectedGeneration(
+ keyArgs, dbKeyInfo);
+ if (resolvedKeyArgs.hasExpectedDataGeneration()) {
+ addRewriteGenerationToAuditMap(
+ resolvedKeyArgs.getExpectedDataGeneration(), auditMap);
+ }
+ return resolvedKeyArgs;
+ }
+
+ /**
+ * Validates the condition at request admission time.
+ *
+ * This returns precondition-style errors because the client condition is
+ * already false before the request is admitted for a serialized commit.
+ */
+ protected void validateAtomicRewriteAtAdmission(OmKeyInfo dbKeyInfo,
+ KeyArgs keyArgs, Map<String, String> auditMap) throws OMException {
+ if (!keyArgs.hasExpectedDataGeneration()) {
+ return;
+ }
+ validateExpectedDataGeneration(dbKeyInfo,
+ keyArgs.getExpectedDataGeneration(), auditMap,
+ AtomicRewritePhase.ADMISSION);
+ }
+
+ /**
+ * Validates an already admitted condition at serialized commit time.
+ *
+ * Mismatches here mean the admission snapshot became stale, so callers
should
+ * surface these as atomic write conflicts.
+ */
+ protected void validateAtomicRewriteAtCommit(OmKeyInfo existing,
+ OmKeyInfo toCommit, Map<String, String> auditMap) throws OMException {
+ validateAtomicRewriteAtCommit(existing,
+ toCommit.getExpectedDataGeneration(), auditMap);
+ }
+
+ /**
+ * Validates an already admitted condition at serialized commit time.
+ *
+ * This form is for callers that must check the admitted generation before
+ * building the final key info to commit.
+ */
+ protected void validateAtomicRewriteAtCommit(OmKeyInfo existing,
+ Long expectedGen, Map<String, String> auditMap) throws OMException {
+ if (expectedGen == null) {
+ return;
+ }
+ validateExpectedDataGeneration(existing, expectedGen, auditMap,
+ AtomicRewritePhase.COMMIT);
+ }
+
+ private void validateExpectedDataGeneration(OmKeyInfo existing,
Review Comment:
I feel like splitting the validation logic into these three methods might be
a bit of over-engineering. Could we simplify it?
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/key/OMKeyRequest.java:
##########
@@ -1308,46 +1308,153 @@ protected void validateEncryptionKeyInfo(OmBucketInfo
bucketInfo, KeyArgs keyArg
}
/**
- * Validates atomic rewrite conditions for conditional writes.
- * <p>
- * For If-None-Match: * (expectedDataGeneration =
EXPECTED_GEN_CREATE_IF_NOT_EXISTS),
- * the key must NOT exist.
- * <p>
- * For If-Match with a specific generation, the key must exist with matching
updateID.
+ * Resolves conditional write state at request admission time.
*
- * @param dbKeyInfo the existing key info from the database (null if key
doesn't exist)
- * @param keyArgs the key arguments containing expected generation
- * @throws OMException if validation fails
+ * This validates expected generations as admission-time preconditions and
+ * rewrites matching ETags into expected generations for commit-time checks.
*/
- protected void validateAtomicRewrite(OmKeyInfo dbKeyInfo, KeyArgs keyArgs)
- throws OMException {
+ protected KeyArgs resolveConditionalWriteAtAdmission(OmKeyInfo dbKeyInfo,
+ KeyArgs keyArgs, Map<String, String> auditMap) throws OMException {
if (keyArgs.hasExpectedDataGeneration()) {
- long expectedGen = keyArgs.getExpectedDataGeneration();
- // If expectedGen is EXPECTED_GEN_CREATE_IF_NOT_EXISTS, it means the key
MUST NOT exist (If-None-Match)
- if (expectedGen == OzoneConsts.EXPECTED_GEN_CREATE_IF_NOT_EXISTS) {
- if (dbKeyInfo != null) {
- throw new OMException("Key already exists",
- OMException.ResultCodes.KEY_ALREADY_EXISTS);
- }
- } else {
- // If a key does not exist, or if it exists but the updateID do not
match, then fail this request.
- if (dbKeyInfo == null) {
- throw new OMException("Key not found during expected rewrite",
- OMException.ResultCodes.KEY_NOT_FOUND);
- }
- if (dbKeyInfo.getUpdateID() != expectedGen) {
- throw new OMException("Generation mismatch during expected rewrite",
- OMException.ResultCodes.KEY_NOT_FOUND);
- }
+ validateAtomicRewriteAtAdmission(dbKeyInfo, keyArgs, auditMap);
+ return keyArgs;
+ }
+
+ KeyArgs resolvedKeyArgs = validateAndRewriteIfMatchAsExpectedGeneration(
+ keyArgs, dbKeyInfo);
+ if (resolvedKeyArgs.hasExpectedDataGeneration()) {
+ addRewriteGenerationToAuditMap(
+ resolvedKeyArgs.getExpectedDataGeneration(), auditMap);
+ }
Review Comment:
A few quick suggestions to streamline the logic:
1. Could we make `validateAndRewriteIfMatchAsExpectedGeneration` private if
it's not used elsewhere?
2. The `keyArgs.hasExpectedDataGeneration()` check inside
`validateAndRewriteIfMatchAsExpectedGeneration` is redundant, as the caller
already filters it out.
3. Consider moving `addRewriteGenerationToAuditMap` into
`validateAndRewriteIfMatchAsExpectedGeneration`. This will make the logic much
cleaner.
--
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]
