Re: [PR] [#2780] Improvement(server,core): Move tree lock from rest api to the corresponding implementation to minimize tree lock range. [gravitino]

[via GitHub]

jerqi commented on code in PR #2873:
URL: https://github.com/apache/gravitino/pull/2873#discussion_r1974930461


##########
core/src/main/java/org/apache/gravitino/authorization/AccessControlManager.java:
##########
@@ -57,78 +61,136 @@ public AccessControlManager(EntityStore store, IdGenerator 
idGenerator, Config c
   @Override
   public User addUser(String metalake, String user)
       throws UserAlreadyExistsException, NoSuchMetalakeException {
-    return userGroupManager.addUser(metalake, user);
+    return TreeLockUtils.doWithTreeLock(
+        
NameIdentifier.of(AuthorizationUtils.ofUserNamespace(metalake).levels()),
+        LockType.WRITE,
+        () -> userGroupManager.addUser(metalake, user));
   }
 
   @Override
   public boolean removeUser(String metalake, String user) throws 
NoSuchMetalakeException {
-    return userGroupManager.removeUser(metalake, user);
+    return TreeLockUtils.doWithTreeLock(
+        
NameIdentifier.of(AuthorizationUtils.ofUserNamespace(metalake).levels()),
+        LockType.WRITE,
+        () -> userGroupManager.removeUser(metalake, user));
   }
 
   @Override
   public User getUser(String metalake, String user)
       throws NoSuchUserException, NoSuchMetalakeException {
-    return userGroupManager.getUser(metalake, user);
+    return TreeLockUtils.doWithTreeLock(
+        AuthorizationUtils.ofUser(metalake, user),
+        LockType.READ,
+        () -> userGroupManager.getUser(metalake, user));
   }
 
   @Override
   public String[] listUserNames(String metalake) throws 
NoSuchMetalakeException {
-    return userGroupManager.listUserNames(metalake);
+    return TreeLockUtils.doWithTreeLock(
+        
NameIdentifier.of(AuthorizationUtils.ofUserNamespace(metalake).levels()),
+        LockType.READ,
+        () -> userGroupManager.listUserNames(metalake));
   }
 
   @Override
   public User[] listUsers(String metalake) throws NoSuchMetalakeException {
-    return userGroupManager.listUsers(metalake);
+    return TreeLockUtils.doWithTreeLock(
+        
NameIdentifier.of(AuthorizationUtils.ofUserNamespace(metalake).levels()),
+        LockType.READ,
+        () -> userGroupManager.listUsers(metalake));
   }
 
   public Group addGroup(String metalake, String group)
       throws GroupAlreadyExistsException, NoSuchMetalakeException {
-    return userGroupManager.addGroup(metalake, group);
+    return TreeLockUtils.doWithTreeLock(
+        
NameIdentifier.of(AuthorizationUtils.ofGroupNamespace(metalake).levels()),
+        LockType.WRITE,
+        () -> userGroupManager.addGroup(metalake, group));
   }
 
   @Override
   public boolean removeGroup(String metalake, String group) throws 
NoSuchMetalakeException {
-    return userGroupManager.removeGroup(metalake, group);
+    return TreeLockUtils.doWithTreeLock(
+        
NameIdentifier.of(AuthorizationUtils.ofGroupNamespace(metalake).levels()),
+        LockType.WRITE,
+        () -> userGroupManager.removeGroup(metalake, group));
   }
 
   @Override
   public Group getGroup(String metalake, String group)
       throws NoSuchGroupException, NoSuchMetalakeException {
-    return userGroupManager.getGroup(metalake, group);
+    return TreeLockUtils.doWithTreeLock(
+        AuthorizationUtils.ofGroup(metalake, group),
+        LockType.READ,
+        () -> userGroupManager.getGroup(metalake, group));
   }
 
   @Override
   public Group[] listGroups(String metalake) throws NoSuchMetalakeException {
-    return userGroupManager.listGroups(metalake);
+    return TreeLockUtils.doWithTreeLock(
+        
NameIdentifier.of(AuthorizationUtils.ofGroupNamespace(metalake).levels()),
+        LockType.READ,
+        () -> userGroupManager.listGroups(metalake));
   }
 
   @Override
   public String[] listGroupNames(String metalake) throws 
NoSuchMetalakeException {
-    return userGroupManager.listGroupNames(metalake);
+    return TreeLockUtils.doWithTreeLock(
+        
NameIdentifier.of(AuthorizationUtils.ofGroupNamespace(metalake).levels()),
+        LockType.READ,
+        () -> userGroupManager.listGroupNames(metalake));
   }
 
   @Override
   public User grantRolesToUser(String metalake, List<String> roles, String 
user)
       throws NoSuchUserException, IllegalRoleException, 
NoSuchMetalakeException {
-    return permissionManager.grantRolesToUser(metalake, roles, user);
+    return TreeLockUtils.doWithTreeLock(
+        AuthorizationUtils.ofUser(metalake, user),
+        LockType.WRITE,
+        () ->
+            TreeLockUtils.doWithTreeLock(
+                
NameIdentifier.of(AuthorizationUtils.ofRoleNamespace(metalake).levels()),

Review Comment:
   I prefer locking role namespace. Because we should align to other code.



-- 
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: commits-unsubscr...@gravitino.apache.org

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