sodonnel commented on code in PR #6385:
URL: https://github.com/apache/ozone/pull/6385#discussion_r1547846561
##########
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:
Any change to the key, including ACLs should change the keys updateID, as
that is what we are relying on to test if the key has changed. That would cause
the optimistic commit to fail.
From the client code perspective, I think its easier to set the ACLs etc at
the time of key open, as otherwise they need to be potentially passed down to
various sub-classes. This also adds different testing paths for each type of
key (Ratis, EC). That is why I am persisting the overwriteUpdateID in the
openKey part. It should be passed as part of open key so we can check they key
has no change since it was read and the key write starts. It costs basically
nothing to persist it in the open key table and saves some complexity on commit.
If you think there are bugs around existing ACLs / Created Time /
Modification time handling then please raise Jiras for them. We cannot change
that as part of this PR, as it would not make sense in this context. A
different PR should take care of that.
On the created time / modification time front - I think could be argued
either way. What is the creation time of a file in Linux? You open the file,
you write a series of bytes of several minutes and close the file. The ctime is
probably the time the file was opened. The mtime is the time the file was last
changed - they can easily be different. With Ozone its not as clear cut as the
key traditionally has not been visible until it is committed. They the
difference between ctime and mtime is really the time it took to write the
bytes. After HBase allows uncommitted keys to be visible, the behavior is more
like Linux and hence is quite possibly correct as it stands.
--
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]
