[GitHub] [ozone] smengcl commented on a diff in pull request #3588: HDDS-6990. AuthorizerLockImpl.java#tryWriteLockInOMRequest: move sani…

[GitBox <gi...@apache.org>]

smengcl commented on code in PR #3588:
URL: https://github.com/apache/ozone/pull/3588#discussion_r919307698


##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/multitenant/AuthorizerLockImpl.java:
##########
@@ -132,11 +132,12 @@ public long tryWriteLockThrowOnTimeout() throws IOException {
   @Override
   public void tryWriteLockInOMRequest() throws IOException {
 
+    long stamp = tryWriteLockThrowOnTimeout();
+
     // Sanity check. Must not have held a write lock in a tenant OMRequest.
     Preconditions.checkArgument(omRequestWriteLockStamp == 0L);

Review Comment:
   > It would make sense to check inside a try and release the lock on failure.
   
   I'm not sure I understand this sentence. If an exception is thrown (e.g. `InterruptedException`), the lock won't be acquired in the first place.
   
   
   > Also, `validateAndUpdateCache` has a pre-condition check which might also land up not unlocking on exception, might make sense to clean up that code as well.
   
   Good catch! `Preconditions.checkXYZ`s in `OMTenantCreateRequest#validateAndUpdateCache` should be moved inside the `try` block. Also we need to double check if other tenant requests have `Preconditions.checkXYZ` outside of `try` block in `validateAndUpdateCache` as well.
   
   We could piggy-back that fix in this PR.



-- 
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: issues-unsubscribe@ozone.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org