Skip to content

Commit

Permalink
Resolve comments.
Browse files Browse the repository at this point in the history
  • Loading branch information
@yuqi1129
yuqi1129 committed Feb 28, 2025
1 parent 6c61f6d commit ab1997e
Show file tree
Hide file tree
Showing 8 changed files with 68 additions and 34 deletions.
24 changes: 4 additions & 20 deletions core/src/main/java/org/apache/gravitino/authorization/AccessControlManager.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -147,11 +147,7 @@ public User grantRolesToUser(String metalake, List<String> 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
Expand All @@ -160,11 +156,7 @@ public Group grantRolesToGroup(String metalake, List<String> 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
Expand All @@ -173,11 +165,7 @@ public Group revokeRolesFromGroup(String metalake, List<String> 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
Expand All @@ -186,11 +174,7 @@ public User revokeRolesFromUser(String metalake, List<String> 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
Expand Down
22 changes: 18 additions & 4 deletions core/src/main/java/org/apache/gravitino/authorization/PermissionManager.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -67,7 +69,10 @@ User grantRolesToUser(String metalake, List<String> roles, String user) {
try {
List<RoleEntity> 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 =
Expand Down Expand Up @@ -153,7 +158,10 @@ Group grantRolesToGroup(String metalake, List<String> roles, String group) {
try {
List<RoleEntity> 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 =
Expand Down Expand Up @@ -239,7 +247,10 @@ Group revokeRolesFromGroup(String metalake, List<String> roles, String group) {
try {
List<RoleEntity> 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 =
Expand Down Expand Up @@ -325,7 +336,10 @@ User revokeRolesFromUser(String metalake, List<String> roles, String user) {
try {
List<RoleEntity> 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 =
Expand Down
17 changes: 14 additions & 3 deletions core/src/main/java/org/apache/gravitino/catalog/CatalogManager.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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);

Expand Down Expand Up @@ -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 =
Expand Down
8 changes: 7 additions & 1 deletion core/src/main/java/org/apache/gravitino/catalog/FilesetOperationDispatcher.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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(
Expand Down
4 changes: 3 additions & 1 deletion core/src/main/java/org/apache/gravitino/catalog/SchemaOperationDispatcher.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down
9 changes: 8 additions & 1 deletion core/src/main/java/org/apache/gravitino/catalog/TableOperationDispatcher.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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 =
Expand Down
3 changes: 2 additions & 1 deletion core/src/main/java/org/apache/gravitino/catalog/TopicOperationDispatcher.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down
15 changes: 12 additions & 3 deletions core/src/main/java/org/apache/gravitino/metalake/MetalakeManager.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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;
Expand Down Expand Up @@ -244,8 +246,7 @@ public BaseMetalake createMetalake(
@Override
public BaseMetalake alterMetalake(NameIdentifier ident, MetalakeChange... changes)
throws NoSuchMetalakeException, IllegalArgumentException {
return TreeLockUtils.doWithRootTreeLock(
LockType.WRITE,
Executable<BaseMetalake, RuntimeException> exceptionExecutable =
() -> {
try {
if (!metalakeInUse(store, ident)) {
Expand Down Expand Up @@ -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
Expand Down

0 comments on commit ab1997e

Please sign in to comment.