errose28 commented on code in PR #6385:
URL: https://github.com/apache/ozone/pull/6385#discussion_r1538383070
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/key/OMKeyCommitRequest.java:
##########
@@ -240,6 +240,13 @@ public OMClientResponse
validateAndUpdateCache(OzoneManager ozoneManager, TermIn
throw new OMException("Failed to " + action + " key, as " + dbOpenKey +
"entry is not found in the OpenKey table", KEY_NOT_FOUND);
}
+
+ validateOptimisticLockingOverwrite(keyToDelete, omKeyInfo, auditMap);
+ // Optimistic locking validation has passed. Now set the overwrite
fields to null so they are
+ // not persisted in the key table.
+ omKeyInfo.setOverwriteUpdateID(null);
+ omKeyInfo.setOverwriteObjectID(null);
Review Comment:
> You could also argue, why persist any of the key meta data on key open
(created date, ACLs, etc).
After looking this over a bit I think these are actually bugs that we need
to fix:
- Create time:
This one is more of a debugging/auditing inconvenience. We set create time
in the create phase before the object is visible, and then set modification
time in the commit phase when the key is visible. Ideally for a key that has
just been created, I would think ctime and mtime would be expected to be the
same. However since both create and commit show up in the audit log I guess you
could argue that the current implementation is correct as well.
- ACLs
This one looks more concerning. I haven't tested this yet, but it looks like
the ACLs at the time of create are what are also committed to the final key,
without checking if the key being replaced had ACL updates in the mean time.
For example:
- key1 exists with acl1
- key1' is created at the same path as key1
- ACLs for key1 are updated to acl2 by another user/admin.
- key1' is committed with acl1 that was read at create time.
Now the ACLs have gone back in time without the admin or user intending to
make this change.
Looking at it from this angle, the existing approach looks like it should be
fixed to write metadata like ACLs and create time at the time of commit. Once
these bugs are fixed, persisting the overwrite update ID at the time of create
does not make much sense in context either.
--
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]
