yuqi1129 commented on code in PR #2873:
URL: https://github.com/apache/gravitino/pull/2873#discussion_r1955695147
##########
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:
Yes, there is no need to put it into the lock, the fact that this part of
logic is between `loadCatalogAndWrap` and `updataCatalog`, unless we split
these two logic and lock them separately can we remove the code out of lock
range.
--
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
