ivandika3 commented on code in PR #9332:
URL: https://github.com/apache/ozone/pull/9332#discussion_r2663688092
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/key/OMKeyCommitRequest.java:
##########
@@ -616,14 +616,23 @@ protected void validateAtomicRewrite(OmKeyInfo existing,
OmKeyInfo toCommit, Map
if (toCommit.getExpectedDataGeneration() != null) {
// These values are not passed in the request keyArgs, so add them into
the auditMap if they are present
// in the open key entry.
- auditMap.put(OzoneConsts.REWRITE_GENERATION,
String.valueOf(toCommit.getExpectedDataGeneration()));
- if (existing == null) {
- throw new OMException("Atomic rewrite is not allowed for a new key",
KEY_NOT_FOUND);
- }
- if
(!toCommit.getExpectedDataGeneration().equals(existing.getUpdateID())) {
- throw new OMException("Cannot commit as current generation (" +
existing.getUpdateID() +
- ") does not match the expected generation to rewrite (" +
toCommit.getExpectedDataGeneration() + ")",
- KEY_NOT_FOUND);
+ Long expectedGen = toCommit.getExpectedDataGeneration();
+ auditMap.put(OzoneConsts.REWRITE_GENERATION,
String.valueOf(expectedGen));
+
+ if (expectedGen == OzoneConsts.EXPECTED_GEN_CREATE_IF_NOT_EXISTS) {
+ if (existing != null) {
+ throw new OMException("Key already exists",
+ OMException.ResultCodes.KEY_ALREADY_EXISTS);
+ }
+ } else {
+ if (existing == null) {
+ throw new OMException("Atomic rewrite is not allowed for a new key",
KEY_NOT_FOUND);
+ }
+ if
(!toCommit.getExpectedDataGeneration().equals(existing.getUpdateID())) {
Review Comment:
Nit: Can reuse `expectedGen`, also applied to the following exception.
`expectedGen` can be pushed before the `if` condition.
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/key/OMKeyCommitRequest.java:
##########
@@ -616,14 +616,23 @@ protected void validateAtomicRewrite(OmKeyInfo existing,
OmKeyInfo toCommit, Map
if (toCommit.getExpectedDataGeneration() != null) {
// These values are not passed in the request keyArgs, so add them into
the auditMap if they are present
// in the open key entry.
- auditMap.put(OzoneConsts.REWRITE_GENERATION,
String.valueOf(toCommit.getExpectedDataGeneration()));
- if (existing == null) {
- throw new OMException("Atomic rewrite is not allowed for a new key",
KEY_NOT_FOUND);
- }
- if
(!toCommit.getExpectedDataGeneration().equals(existing.getUpdateID())) {
- throw new OMException("Cannot commit as current generation (" +
existing.getUpdateID() +
- ") does not match the expected generation to rewrite (" +
toCommit.getExpectedDataGeneration() + ")",
- KEY_NOT_FOUND);
+ Long expectedGen = toCommit.getExpectedDataGeneration();
+ auditMap.put(OzoneConsts.REWRITE_GENERATION,
String.valueOf(expectedGen));
+
+ if (expectedGen == OzoneConsts.EXPECTED_GEN_CREATE_IF_NOT_EXISTS) {
+ if (existing != null) {
+ throw new OMException("Key already exists",
+ OMException.ResultCodes.KEY_ALREADY_EXISTS);
+ }
Review Comment:
Could you help check whether it's better to use a new `ResultCodes` specific
to return S3 `412 Precondition Failed`? Although `KEY_ALREADY_EXISTS` does not
seem to be handled by S3G currently, it might be safer to add a new result code
just in case `KEY_ALREADY_EXISTS` will be used for other S3 operations.
--
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]
