jerryshao commented on code in PR #2873:
URL: https://github.com/apache/gravitino/pull/2873#discussion_r1973458010
##########
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 think it should `role`, not `role namespace`. WDYT?
##########
core/src/main/java/org/apache/gravitino/catalog/SchemaOperationDispatcher.java:
##########
@@ -201,68 +213,73 @@ public Schema loadSchema(NameIdentifier ident) throws
NoSuchSchemaException {
@Override
public Schema alterSchema(NameIdentifier ident, SchemaChange... changes)
throws NoSuchSchemaException {
- validateAlterProperties(ident,
HasPropertyMetadata::schemaPropertiesMetadata, changes);
NameIdentifier catalogIdent = getCatalogIdentifier(ident);
- Schema alteredSchema =
- doWithCatalog(
- catalogIdent,
- c -> c.doWithSchemaOps(s -> s.alterSchema(ident, changes)),
- NoSuchSchemaException.class);
-
- // If the Schema is maintained by the Gravitino's store, we don't have to
alter again.
- boolean isManagedSchema = isManagedEntity(catalogIdent,
Capability.Scope.SCHEMA);
- if (isManagedSchema) {
- return EntityCombinedSchema.of(alteredSchema)
- .withHiddenProperties(
- getHiddenPropertyNames(
+ return TreeLockUtils.doWithTreeLock(
+ NameIdentifier.of(ident.namespace().levels()),
Review Comment:
I'm curious why do we need to add the write in the catalog level? I think it
should be fine to add the write lock in the schema level, am I right?
##########
core/src/main/java/org/apache/gravitino/catalog/TableOperationDispatcher.java:
##########
@@ -194,70 +198,75 @@ public Table createTable(
public Table alterTable(NameIdentifier ident, TableChange... changes)
throws NoSuchTableException, IllegalArgumentException {
validateAlterProperties(ident,
HasPropertyMetadata::tablePropertiesMetadata, changes);
+ return TreeLockUtils.doWithTreeLock(
+ NameIdentifier.of(ident.namespace().levels()),
+ LockType.WRITE,
Review Comment:
Also here, why alter operation needs parent write lock?
##########
core/src/main/java/org/apache/gravitino/catalog/CatalogManager.java:
##########
@@ -589,124 +627,137 @@ public void disableCatalog(NameIdentifier ident) throws
NoSuchCatalogException {
@Override
public Catalog alterCatalog(NameIdentifier ident, CatalogChange... changes)
throws NoSuchCatalogException, IllegalArgumentException {
- checkCatalogInUse(store, ident);
-
- // There could be a race issue that someone is using the catalog from
cache while we are
- // updating it.
-
- CatalogWrapper catalogWrapper = loadCatalogAndWrap(ident);
- if (catalogWrapper == null) {
- throw new NoSuchCatalogException(CATALOG_DOES_NOT_EXIST_MSG, ident);
- }
-
- try {
- catalogWrapper.doWithPropertiesMeta(
- f -> {
- Pair<Map<String, String>, Map<String, String>> alterProperty =
- getCatalogAlterProperty(changes);
- validatePropertyForAlter(
- f.catalogPropertiesMetadata(), alterProperty.getLeft(),
alterProperty.getRight());
- return null;
- });
- } catch (IllegalArgumentException e1) {
- throw e1;
- } catch (Exception e) {
- LOG.error("Failed to alter catalog {}", ident, e);
- throw new RuntimeException(e);
- }
-
- catalogCache.invalidate(ident);
- try {
- CatalogEntity updatedCatalog =
- store.update(
- ident,
- CatalogEntity.class,
- EntityType.CATALOG,
- catalog -> {
- CatalogEntity.Builder newCatalogBuilder =
- newCatalogBuilder(ident.namespace(), catalog);
-
- Map<String, String> newProps =
- catalog.getProperties() == null
- ? new HashMap<>()
- : new HashMap<>(catalog.getProperties());
- newCatalogBuilder = updateEntity(newCatalogBuilder, newProps,
changes);
-
- return newCatalogBuilder.build();
- });
- return Objects.requireNonNull(
- catalogCache.get(
- updatedCatalog.nameIdentifier(),
- id -> createCatalogWrapper(updatedCatalog, null)))
- .catalog;
-
- } catch (NoSuchEntityException ne) {
- LOG.warn("Catalog {} does not exist", ident, ne);
- throw new NoSuchCatalogException(CATALOG_DOES_NOT_EXIST_MSG, ident);
-
- } catch (IllegalArgumentException iae) {
- LOG.warn("Failed to alter catalog {} with unknown change", ident, iae);
- throw iae;
-
- } catch (IOException ioe) {
- LOG.error("Failed to alter catalog {}", ident, ioe);
- throw new RuntimeException(ioe);
- }
+ return TreeLockUtils.doWithTreeLock(
+ NameIdentifier.of(ident.namespace().level(0)),
+ LockType.WRITE,
+ () -> {
+ checkCatalogInUse(store, ident);
+
+ // There could be a race issue that someone is using the catalog
from cache while we are
+ // updating it.
+
+ CatalogWrapper catalogWrapper = loadCatalogAndWrap(ident);
+ if (catalogWrapper == null) {
+ throw new NoSuchCatalogException(CATALOG_DOES_NOT_EXIST_MSG,
ident);
+ }
+
+ try {
+ catalogWrapper.doWithPropertiesMeta(
+ f -> {
+ Pair<Map<String, String>, Map<String, String>> alterProperty
=
+ getCatalogAlterProperty(changes);
+ validatePropertyForAlter(
+ f.catalogPropertiesMetadata(),
+ alterProperty.getLeft(),
+ alterProperty.getRight());
+ return null;
+ });
+ } catch (IllegalArgumentException e1) {
+ throw e1;
+ } catch (Exception e) {
+ LOG.error("Failed to alter catalog {}", ident, e);
+ throw new RuntimeException(e);
+ }
Review Comment:
I think we can split into two locks, it should be fine.
##########
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()),
+ LockType.READ,
+ () -> permissionManager.grantRolesToUser(metalake, roles,
user)));
}
@Override
public Group grantRolesToGroup(String metalake, List<String> roles, String
group)
throws NoSuchGroupException, IllegalRoleException,
NoSuchMetalakeException {
- return permissionManager.grantRolesToGroup(metalake, roles, group);
+ return TreeLockUtils.doWithTreeLock(
+ AuthorizationUtils.ofGroup(metalake, group),
+ LockType.WRITE,
+ () ->
+ TreeLockUtils.doWithTreeLock(
+
NameIdentifier.of(AuthorizationUtils.ofRoleNamespace(metalake).levels()),
Review Comment:
Also here.
--
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
