jerryshao commented on code in PR #2873:
URL: https://github.com/apache/gravitino/pull/2873#discussion_r1952091523
##########
core/src/main/java/org/apache/gravitino/authorization/AccessControlManager.java:
##########
@@ -57,44 +61,68 @@ 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.ofGroupNamespace(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.ofGroupNamespace(metalake).levels()),
Review Comment:
Also here.
##########
core/src/main/java/org/apache/gravitino/authorization/AccessControlManager.java:
##########
@@ -143,42 +199,60 @@ public Role createRole(
Map<String, String> properties,
List<SecurableObject> securableObjects)
throws RoleAlreadyExistsException, NoSuchMetalakeException {
- return roleManager.createRole(metalake, role, properties,
securableObjects);
+ return TreeLockUtils.doWithTreeLock(
+
NameIdentifier.of(AuthorizationUtils.ofRoleNamespace(metalake).levels()),
+ LockType.WRITE,
+ () -> roleManager.createRole(metalake, role, properties,
securableObjects));
}
@Override
public Role getRole(String metalake, String role)
throws NoSuchRoleException, NoSuchMetalakeException {
- return roleManager.getRole(metalake, role);
+ return TreeLockUtils.doWithTreeLock(
+ AuthorizationUtils.ofRole(metalake, role),
+ LockType.READ,
+ () -> roleManager.getRole(metalake, role));
}
@Override
public boolean deleteRole(String metalake, String role) throws
NoSuchMetalakeException {
- return roleManager.deleteRole(metalake, role);
+ return TreeLockUtils.doWithTreeLock(
+
NameIdentifier.of(AuthorizationUtils.ofRoleNamespace(metalake).levels()),
+ LockType.WRITE,
+ () -> roleManager.deleteRole(metalake, role));
}
@Override
public String[] listRoleNames(String metalake) throws
NoSuchMetalakeException {
- return roleManager.listRoleNames(metalake);
+ return TreeLockUtils.doWithTreeLock(
+ NameIdentifier.of(metalake), LockType.READ, () ->
roleManager.listRoleNames(metalake));
Review Comment:
Should we use the role namespace like above?
##########
core/src/main/java/org/apache/gravitino/catalog/CatalogManager.java:
##########
@@ -466,115 +485,134 @@ public void testConnection(
String comment,
Map<String, String> properties) {
NameIdentifier metalakeIdent =
NameIdentifier.of(ident.namespace().levels());
- checkMetalake(metalakeIdent, store);
- try {
- if (store.exists(ident, EntityType.CATALOG)) {
- throw new CatalogAlreadyExistsException("Catalog %s already exists",
ident);
- }
+ TreeLockUtils.doWithTreeLock(
+ metalakeIdent,
+ LockType.READ,
+ () -> {
+ checkMetalake(metalakeIdent, store);
+ try {
+ if (store.exists(ident, EntityType.CATALOG)) {
+ throw new CatalogAlreadyExistsException("Catalog %s already
exists", ident);
+ }
- Map<String, String> mergedConfig = buildCatalogConf(provider,
properties);
- Instant now = Instant.now();
- String creator = PrincipalUtils.getCurrentPrincipal().getName();
- CatalogEntity dummyEntity =
- CatalogEntity.builder()
- .withId(DUMMY_ID.id())
- .withName(ident.name())
- .withNamespace(ident.namespace())
- .withType(type)
- .withProvider(provider)
- .withComment(comment)
- .withProperties(StringIdentifier.newPropertiesWithId(DUMMY_ID,
mergedConfig))
- .withAuditInfo(
- AuditInfo.builder()
- .withCreator(creator)
- .withCreateTime(now)
- .withLastModifier(creator)
- .withLastModifiedTime(now)
- .build())
- .build();
-
- CatalogWrapper wrapper = createCatalogWrapper(dummyEntity, mergedConfig);
- wrapper.doWithCatalogOps(
- c -> {
- c.testConnection(ident, type, provider, comment, mergedConfig);
- return null;
- });
- } catch (GravitinoRuntimeException e) {
- throw e;
- } catch (Exception e) {
- LOG.warn("Failed to test catalog creation {}", ident, e);
- if (e instanceof RuntimeException) {
- throw (RuntimeException) e;
- }
- throw new RuntimeException(e);
- }
+ Map<String, String> mergedConfig = buildCatalogConf(provider,
properties);
+ Instant now = Instant.now();
+ String creator = PrincipalUtils.getCurrentPrincipal().getName();
+ CatalogEntity dummyEntity =
+ CatalogEntity.builder()
+ .withId(DUMMY_ID.id())
+ .withName(ident.name())
+ .withNamespace(ident.namespace())
+ .withType(type)
+ .withProvider(provider)
+ .withComment(comment)
+
.withProperties(StringIdentifier.newPropertiesWithId(DUMMY_ID, mergedConfig))
+ .withAuditInfo(
+ AuditInfo.builder()
+ .withCreator(creator)
+ .withCreateTime(now)
+ .withLastModifier(creator)
+ .withLastModifiedTime(now)
+ .build())
+ .build();
+
+ CatalogWrapper wrapper = createCatalogWrapper(dummyEntity,
mergedConfig);
+ return wrapper.doWithCatalogOps(
+ c -> {
+ c.testConnection(ident, type, provider, comment,
mergedConfig);
+ return null;
+ });
+ } catch (GravitinoRuntimeException e) {
+ throw e;
+ } catch (Exception e) {
+ LOG.warn("Failed to test catalog creation {}", ident, e);
+ if (e instanceof RuntimeException) {
+ throw (RuntimeException) e;
+ }
+ throw new RuntimeException(e);
+ }
+ });
Review Comment:
Do we need add lock for this part, I feel it is not so necessary, what do
you think? @mchades
##########
core/src/main/java/org/apache/gravitino/authorization/AccessControlManager.java:
##########
@@ -57,44 +61,68 @@ 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.ofGroupNamespace(metalake).levels()),
Review Comment:
IIUC, it should be `UserNamespace`, not `GroupNamespace`, right?
##########
core/src/main/java/org/apache/gravitino/authorization/AccessControlManager.java:
##########
@@ -57,44 +61,68 @@ 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.ofGroupNamespace(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.ofGroupNamespace(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.ofGroup(metalake, user),
Review Comment:
Also here.
##########
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:
Do we need to add this part into the lock?
--
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
