diff --git a/core/src/main/java/org/apache/gravitino/authorization/AccessControlManager.java b/core/src/main/java/org/apache/gravitino/authorization/AccessControlManager.java index 8a6934a3a43..aa7476534e8 100644 --- a/core/src/main/java/org/apache/gravitino/authorization/AccessControlManager.java +++ b/core/src/main/java/org/apache/gravitino/authorization/AccessControlManager.java @@ -147,11 +147,7 @@ public User grantRolesToUser(String metalake, List roles, String 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))); + () -> permissionManager.grantRolesToUser(metalake, roles, user)); } @Override @@ -160,11 +156,7 @@ public Group grantRolesToGroup(String metalake, List roles, String group return TreeLockUtils.doWithTreeLock( AuthorizationUtils.ofGroup(metalake, group), LockType.WRITE, - () -> - TreeLockUtils.doWithTreeLock( - NameIdentifier.of(AuthorizationUtils.ofRoleNamespace(metalake).levels()), - LockType.READ, - () -> permissionManager.grantRolesToGroup(metalake, roles, group))); + () -> permissionManager.grantRolesToGroup(metalake, roles, group)); } @Override @@ -173,11 +165,7 @@ public Group revokeRolesFromGroup(String metalake, List roles, String gr return TreeLockUtils.doWithTreeLock( AuthorizationUtils.ofGroup(metalake, group), LockType.WRITE, - () -> - TreeLockUtils.doWithTreeLock( - NameIdentifier.of(AuthorizationUtils.ofRoleNamespace(metalake).levels()), - LockType.READ, - () -> permissionManager.revokeRolesFromGroup(metalake, roles, group))); + () -> permissionManager.revokeRolesFromGroup(metalake, roles, group)); } @Override @@ -186,11 +174,7 @@ public User revokeRolesFromUser(String metalake, List roles, String user return TreeLockUtils.doWithTreeLock( AuthorizationUtils.ofUser(metalake, user), LockType.WRITE, - () -> - TreeLockUtils.doWithTreeLock( - NameIdentifier.of(AuthorizationUtils.ofRoleNamespace(metalake).levels()), - LockType.READ, - () -> permissionManager.revokeRolesFromUser(metalake, roles, user))); + () -> permissionManager.revokeRolesFromUser(metalake, roles, user)); } @Override diff --git a/core/src/main/java/org/apache/gravitino/authorization/PermissionManager.java b/core/src/main/java/org/apache/gravitino/authorization/PermissionManager.java index 1046723d706..60fac44fd83 100644 --- a/core/src/main/java/org/apache/gravitino/authorization/PermissionManager.java +++ b/core/src/main/java/org/apache/gravitino/authorization/PermissionManager.java @@ -39,6 +39,8 @@ import org.apache.gravitino.exceptions.NoSuchGroupException; import org.apache.gravitino.exceptions.NoSuchRoleException; import org.apache.gravitino.exceptions.NoSuchUserException; +import org.apache.gravitino.lock.LockType; +import org.apache.gravitino.lock.TreeLockUtils; import org.apache.gravitino.meta.AuditInfo; import org.apache.gravitino.meta.GroupEntity; import org.apache.gravitino.meta.RoleEntity; @@ -67,7 +69,10 @@ User grantRolesToUser(String metalake, List roles, String user) { try { List roleEntitiesToGrant = Lists.newArrayList(); for (String role : roles) { - roleEntitiesToGrant.add(roleManager.getRole(metalake, role)); + TreeLockUtils.doWithTreeLock( + AuthorizationUtils.ofRole(metalake, role), + LockType.READ, + () -> roleEntitiesToGrant.add(roleManager.getRole(metalake, role))); } User updatedUser = @@ -153,7 +158,10 @@ Group grantRolesToGroup(String metalake, List roles, String group) { try { List roleEntitiesToGrant = Lists.newArrayList(); for (String role : roles) { - roleEntitiesToGrant.add(roleManager.getRole(metalake, role)); + TreeLockUtils.doWithTreeLock( + AuthorizationUtils.ofRole(metalake, role), + LockType.READ, + () -> roleEntitiesToGrant.add(roleManager.getRole(metalake, role))); } Group updatedGroup = @@ -239,7 +247,10 @@ Group revokeRolesFromGroup(String metalake, List roles, String group) { try { List roleEntitiesToRevoke = Lists.newArrayList(); for (String role : roles) { - roleEntitiesToRevoke.add(roleManager.getRole(metalake, role)); + TreeLockUtils.doWithTreeLock( + AuthorizationUtils.ofRole(metalake, role), + LockType.READ, + () -> roleEntitiesToRevoke.add(roleManager.getRole(metalake, role))); } Group updatedGroup = @@ -325,7 +336,10 @@ User revokeRolesFromUser(String metalake, List roles, String user) { try { List roleEntitiesToRevoke = Lists.newArrayList(); for (String role : roles) { - roleEntitiesToRevoke.add(roleManager.getRole(metalake, role)); + TreeLockUtils.doWithTreeLock( + AuthorizationUtils.ofRole(metalake, role), + LockType.READ, + () -> roleEntitiesToRevoke.add(roleManager.getRole(metalake, role))); } User updatedUser = diff --git a/core/src/main/java/org/apache/gravitino/catalog/CatalogManager.java b/core/src/main/java/org/apache/gravitino/catalog/CatalogManager.java index ec30a879fd3..7c1722c1cef 100644 --- a/core/src/main/java/org/apache/gravitino/catalog/CatalogManager.java +++ b/core/src/main/java/org/apache/gravitino/catalog/CatalogManager.java @@ -622,9 +622,9 @@ public void disableCatalog(NameIdentifier ident) throws NoSuchCatalogException { @Override public Catalog alterCatalog(NameIdentifier ident, CatalogChange... changes) throws NoSuchCatalogException, IllegalArgumentException { - return TreeLockUtils.doWithTreeLock( - NameIdentifier.of(ident.namespace().level(0)), - LockType.WRITE, + TreeLockUtils.doWithTreeLock( + ident, + LockType.READ, () -> { checkCatalogInUse(store, ident); @@ -653,7 +653,18 @@ public Catalog alterCatalog(NameIdentifier ident, CatalogChange... changes) LOG.error("Failed to alter catalog {}", ident, e); throw new RuntimeException(e); } + return null; + }); + + boolean containsRenameCatalog = + Arrays.stream(changes).anyMatch(c -> c instanceof CatalogChange.RenameCatalog); + NameIdentifier nameIdentifierForLock = + containsRenameCatalog ? NameIdentifier.of(ident.namespace().level(0)) : ident; + return TreeLockUtils.doWithTreeLock( + nameIdentifierForLock, + LockType.WRITE, + () -> { catalogCache.invalidate(ident); try { CatalogEntity updatedCatalog = diff --git a/core/src/main/java/org/apache/gravitino/catalog/FilesetOperationDispatcher.java b/core/src/main/java/org/apache/gravitino/catalog/FilesetOperationDispatcher.java index 7f37c8da0c4..ff1e8b6adca 100644 --- a/core/src/main/java/org/apache/gravitino/catalog/FilesetOperationDispatcher.java +++ b/core/src/main/java/org/apache/gravitino/catalog/FilesetOperationDispatcher.java @@ -21,6 +21,7 @@ import static org.apache.gravitino.catalog.PropertiesMetadataHelpers.validatePropertyForCreate; import static org.apache.gravitino.utils.NameIdentifierUtil.getCatalogIdentifier; +import java.util.Arrays; import java.util.Map; import org.apache.gravitino.EntityStore; import org.apache.gravitino.NameIdentifier; @@ -180,9 +181,14 @@ public Fileset alterFileset(NameIdentifier ident, FilesetChange... changes) validateAlterProperties(ident, HasPropertyMetadata::filesetPropertiesMetadata, changes); NameIdentifier catalogIdent = getCatalogIdentifier(ident); + boolean containsRenameFileset = + Arrays.stream(changes).anyMatch(c -> c instanceof FilesetChange.RenameFileset); + NameIdentifier nameIdentifierForLock = + containsRenameFileset ? NameIdentifier.of(ident.namespace().levels()) : ident; + Fileset alteredFileset = TreeLockUtils.doWithTreeLock( - NameIdentifier.of(ident.namespace().levels()), + nameIdentifierForLock, LockType.WRITE, () -> doWithCatalog( diff --git a/core/src/main/java/org/apache/gravitino/catalog/SchemaOperationDispatcher.java b/core/src/main/java/org/apache/gravitino/catalog/SchemaOperationDispatcher.java index 056f0fff63f..b9f382e46ae 100644 --- a/core/src/main/java/org/apache/gravitino/catalog/SchemaOperationDispatcher.java +++ b/core/src/main/java/org/apache/gravitino/catalog/SchemaOperationDispatcher.java @@ -215,8 +215,10 @@ public Schema alterSchema(NameIdentifier ident, SchemaChange... changes) throws NoSuchSchemaException { NameIdentifier catalogIdent = getCatalogIdentifier(ident); + // Gravitino does not support alter schema currently, so we do not need to check whether there + // exists SchemaChange.renameSchema in the changes and can lock schema directly. return TreeLockUtils.doWithTreeLock( - NameIdentifier.of(ident.namespace().levels()), + ident, LockType.WRITE, () -> { validateAlterProperties(ident, HasPropertyMetadata::schemaPropertiesMetadata, changes); diff --git a/core/src/main/java/org/apache/gravitino/catalog/TableOperationDispatcher.java b/core/src/main/java/org/apache/gravitino/catalog/TableOperationDispatcher.java index 3c84b91d3ac..de3fdc21f9a 100644 --- a/core/src/main/java/org/apache/gravitino/catalog/TableOperationDispatcher.java +++ b/core/src/main/java/org/apache/gravitino/catalog/TableOperationDispatcher.java @@ -198,9 +198,16 @@ public Table createTable( public Table alterTable(NameIdentifier ident, TableChange... changes) throws NoSuchTableException, IllegalArgumentException { validateAlterProperties(ident, HasPropertyMetadata::tablePropertiesMetadata, changes); + + // Check if there exist TableChange.RenameTable in the changes, if so, we need to TreeLock of + // write on the new table name, or use the read lock on the table instead. + boolean containsRenameTable = + Arrays.stream(changes).anyMatch(c -> c instanceof TableChange.RenameTable); + LockType lockType = containsRenameTable ? LockType.WRITE : LockType.READ; + return TreeLockUtils.doWithTreeLock( NameIdentifier.of(ident.namespace().levels()), - LockType.WRITE, + lockType, () -> { NameIdentifier catalogIdent = getCatalogIdentifier(ident); Table alteredTable = diff --git a/core/src/main/java/org/apache/gravitino/catalog/TopicOperationDispatcher.java b/core/src/main/java/org/apache/gravitino/catalog/TopicOperationDispatcher.java index c56fc283b4f..7ecc1107ff0 100644 --- a/core/src/main/java/org/apache/gravitino/catalog/TopicOperationDispatcher.java +++ b/core/src/main/java/org/apache/gravitino/catalog/TopicOperationDispatcher.java @@ -159,8 +159,9 @@ public Topic alterTopic(NameIdentifier ident, TopicChange... changes) validateAlterProperties(ident, HasPropertyMetadata::topicPropertiesMetadata, changes); + // As Gravitino does not support TopicChange.renameTopic, we can directly lock the topic. return TreeLockUtils.doWithTreeLock( - NameIdentifier.of(ident.namespace().levels()), + ident, LockType.WRITE, () -> { NameIdentifier catalogIdent = getCatalogIdentifier(ident); diff --git a/core/src/main/java/org/apache/gravitino/metalake/MetalakeManager.java b/core/src/main/java/org/apache/gravitino/metalake/MetalakeManager.java index 6f2809e17fb..c1b464f4368 100644 --- a/core/src/main/java/org/apache/gravitino/metalake/MetalakeManager.java +++ b/core/src/main/java/org/apache/gravitino/metalake/MetalakeManager.java @@ -23,6 +23,7 @@ import com.google.common.collect.Maps; import java.io.IOException; import java.time.Instant; +import java.util.Arrays; import java.util.HashMap; import java.util.List; import java.util.Map; @@ -47,6 +48,7 @@ import org.apache.gravitino.meta.CatalogEntity; import org.apache.gravitino.meta.SchemaVersion; import org.apache.gravitino.storage.IdGenerator; +import org.apache.gravitino.utils.Executable; import org.apache.gravitino.utils.PrincipalUtils; import org.slf4j.Logger; import org.slf4j.LoggerFactory; @@ -244,8 +246,7 @@ public BaseMetalake createMetalake( @Override public BaseMetalake alterMetalake(NameIdentifier ident, MetalakeChange... changes) throws NoSuchMetalakeException, IllegalArgumentException { - return TreeLockUtils.doWithRootTreeLock( - LockType.WRITE, + Executable exceptionExecutable = () -> { try { if (!metalakeInUse(store, ident)) { @@ -280,7 +281,15 @@ public BaseMetalake alterMetalake(NameIdentifier ident, MetalakeChange... change LOG.error("Loading Metalake {} failed due to storage issues", ident, ioe); throw new RuntimeException(ioe); } - }); + }; + + boolean containsRenameMetalake = + Arrays.stream(changes).anyMatch(c -> c instanceof MetalakeChange.RenameMetalake); + if (containsRenameMetalake) { + return TreeLockUtils.doWithRootTreeLock(LockType.WRITE, exceptionExecutable); + } + + return TreeLockUtils.doWithTreeLock(ident, LockType.WRITE, exceptionExecutable); } @Override