This is an automated email from the ASF dual-hosted git repository.
jshao pushed a commit to branch main
in repository https://gitbox.apache.org/repos/asf/gravitino.git
The following commit(s) were added to refs/heads/main by this push:
new e8241a967 [#6044] improve(lock): optimization tree lock when drop and
load Table/Schema (#6063)
e8241a967 is described below
commit e8241a96701a2e135db31a94474e3758464f4427
Author: Xun <[email protected]>
AuthorDate: Fri Jan 3 15:58:20 2025 +0800
[#6044] improve(lock): optimization tree lock when drop and load
Table/Schema (#6063)
### What changes were proposed in this pull request?
Modify Schema and Table RESTful interface lock operations.
### Why are the changes needed?
Fix: #6044
### Does this PR introduce _any_ user-facing change?
N/A
### How was this patch tested?
CI Passed.
---
.../catalog/SchemaOperationDispatcher.java | 64 +++++-----
.../catalog/TableOperationDispatcher.java | 131 ++++++++++++---------
.../apache/gravitino/utils/NameIdentifierUtil.java | 31 ++++-
.../server/web/rest/SchemaOperations.java | 6 +-
.../gravitino/server/web/rest/TableOperations.java | 6 +-
5 files changed, 139 insertions(+), 99 deletions(-)
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 789e5e471..8f36ce0d9 100644
---
a/core/src/main/java/org/apache/gravitino/catalog/SchemaOperationDispatcher.java
+++
b/core/src/main/java/org/apache/gravitino/catalog/SchemaOperationDispatcher.java
@@ -277,36 +277,40 @@ public class SchemaOperationDispatcher extends
OperationDispatcher implements Sc
@Override
public boolean dropSchema(NameIdentifier ident, boolean cascade) throws
NonEmptySchemaException {
NameIdentifier catalogIdent = getCatalogIdentifier(ident);
- boolean droppedFromCatalog =
- doWithCatalog(
- catalogIdent,
- c -> c.doWithSchemaOps(s -> s.dropSchema(ident, cascade)),
- NonEmptySchemaException.class,
- RuntimeException.class);
-
- // For managed schema, we don't need to drop the schema from the store
again.
- boolean isManagedSchema = isManagedEntity(catalogIdent,
Capability.Scope.SCHEMA);
- if (isManagedSchema) {
- return droppedFromCatalog;
- }
-
- // For unmanaged schema, it could happen that the schema:
- // 1. Is not found in the catalog (dropped directly from underlying
sources)
- // 2. Is found in the catalog but not in the store (not managed by
Gravitino)
- // 3. Is found in the catalog and the store (managed by Gravitino)
- // 4. Neither found in the catalog nor in the store.
- // In all situations, we try to delete the schema from the store, but we
don't take the
- // return value of the store operation into account. We only take the
return value of the
- // catalog into account.
- try {
- store.delete(ident, SCHEMA, cascade);
- } catch (NoSuchEntityException e) {
- LOG.warn("The schema to be dropped does not exist in the store: {}",
ident, e);
- } catch (Exception e) {
- throw new RuntimeException(e);
- }
-
- return droppedFromCatalog;
+ return TreeLockUtils.doWithTreeLock(
+ catalogIdent,
+ LockType.WRITE,
+ () -> {
+ boolean droppedFromCatalog =
+ doWithCatalog(
+ catalogIdent,
+ c -> c.doWithSchemaOps(s -> s.dropSchema(ident, cascade)),
+ NonEmptySchemaException.class,
+ RuntimeException.class);
+
+ // For managed schema, we don't need to drop the schema from the
store again.
+ boolean isManagedSchema = isManagedEntity(catalogIdent,
Capability.Scope.SCHEMA);
+ if (isManagedSchema) {
+ return droppedFromCatalog;
+ }
+
+ // For unmanaged schema, it could happen that the schema:
+ // 1. Is not found in the catalog (dropped directly from underlying
sources)
+ // 2. Is found in the catalog but not in the store (not managed by
Gravitino)
+ // 3. Is found in the catalog and the store (managed by Gravitino)
+ // 4. Neither found in the catalog nor in the store.
+ // In all situations, we try to delete the schema from the store,
but we don't take the
+ // return value of the store operation into account. We only take
the return value of the
+ // catalog into account.
+ try {
+ store.delete(ident, SCHEMA, cascade);
+ } catch (NoSuchEntityException e) {
+ LOG.warn("The schema to be dropped does not exist in the store:
{}", ident, e);
+ } catch (Exception e) {
+ throw new RuntimeException(e);
+ }
+ return droppedFromCatalog;
+ });
}
private void importSchema(NameIdentifier identifier) {
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 7a4c5a565..3e6aa2abb 100644
---
a/core/src/main/java/org/apache/gravitino/catalog/TableOperationDispatcher.java
+++
b/core/src/main/java/org/apache/gravitino/catalog/TableOperationDispatcher.java
@@ -62,6 +62,7 @@ import
org.apache.gravitino.rel.expressions.transforms.Transform;
import org.apache.gravitino.rel.indexes.Index;
import org.apache.gravitino.rel.indexes.Indexes;
import org.apache.gravitino.storage.IdGenerator;
+import org.apache.gravitino.utils.NameIdentifierUtil;
import org.apache.gravitino.utils.PrincipalUtils;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
@@ -269,33 +270,41 @@ public class TableOperationDispatcher extends
OperationDispatcher implements Tab
*/
@Override
public boolean dropTable(NameIdentifier ident) {
- NameIdentifier catalogIdent = getCatalogIdentifier(ident);
- boolean droppedFromCatalog =
- doWithCatalog(
- catalogIdent, c -> c.doWithTableOps(t -> t.dropTable(ident)),
RuntimeException.class);
-
- // For unmanaged table, it could happen that the table:
- // 1. Is not found in the catalog (dropped directly from underlying
sources)
- // 2. Is found in the catalog but not in the store (not managed by
Gravitino)
- // 3. Is found in the catalog and the store (managed by Gravitino)
- // 4. Neither found in the catalog nor in the store.
- // In all situations, we try to delete the schema from the store, but we
don't take the
- // return value of the store operation into account. We only take the
return value of the
- // catalog into account.
- //
- // For managed table, we should take the return value of the store
operation into account.
- boolean droppedFromStore = false;
- try {
- droppedFromStore = store.delete(ident, TABLE);
- } catch (NoSuchEntityException e) {
- LOG.warn("The table to be dropped does not exist in the store: {}",
ident, e);
- } catch (Exception e) {
- throw new RuntimeException(e);
- }
-
- return isManagedEntity(catalogIdent, Capability.Scope.TABLE)
- ? droppedFromStore
- : droppedFromCatalog;
+ NameIdentifier schemaIdentifier =
NameIdentifierUtil.getSchemaIdentifier(ident);
+ return TreeLockUtils.doWithTreeLock(
+ schemaIdentifier,
+ LockType.WRITE,
+ () -> {
+ NameIdentifier catalogIdent = getCatalogIdentifier(ident);
+ boolean droppedFromCatalog =
+ doWithCatalog(
+ catalogIdent,
+ c -> c.doWithTableOps(t -> t.dropTable(ident)),
+ RuntimeException.class);
+
+ // For unmanaged table, it could happen that the table:
+ // 1. Is not found in the catalog (dropped directly from underlying
sources)
+ // 2. Is found in the catalog but not in the store (not managed by
Gravitino)
+ // 3. Is found in the catalog and the store (managed by Gravitino)
+ // 4. Neither found in the catalog nor in the store.
+ // In all situations, we try to delete the schema from the store,
but we don't take the
+ // return value of the store operation into account. We only take
the return value of the
+ // catalog into account.
+ //
+ // For managed table, we should take the return value of the store
operation into account.
+ boolean droppedFromStore = false;
+ try {
+ droppedFromStore = store.delete(ident, TABLE);
+ } catch (NoSuchEntityException e) {
+ LOG.warn("The table to be dropped does not exist in the store:
{}", ident, e);
+ } catch (Exception e) {
+ throw new RuntimeException(e);
+ }
+
+ return isManagedEntity(catalogIdent, Capability.Scope.TABLE)
+ ? droppedFromStore
+ : droppedFromCatalog;
+ });
}
/**
@@ -314,37 +323,43 @@ public class TableOperationDispatcher extends
OperationDispatcher implements Tab
*/
@Override
public boolean purgeTable(NameIdentifier ident) throws
UnsupportedOperationException {
- NameIdentifier catalogIdent = getCatalogIdentifier(ident);
- boolean droppedFromCatalog =
- doWithCatalog(
- catalogIdent,
- c -> c.doWithTableOps(t -> t.purgeTable(ident)),
- RuntimeException.class,
- UnsupportedOperationException.class);
-
- // For unmanaged table, it could happen that the table:
- // 1. Is not found in the catalog (dropped directly from underlying
sources)
- // 2. Is found in the catalog but not in the store (not managed by
Gravitino)
- // 3. Is found in the catalog and the store (managed by Gravitino)
- // 4. Neither found in the catalog nor in the store.
- // In all situations, we try to delete the schema from the store, but we
don't take the
- // return value of the store operation into account. We only take the
return value of the
- // catalog into account.
- //
- // For managed table, we should take the return value of the store
operation into account.
- boolean droppedFromStore = false;
- try {
- droppedFromStore = store.delete(ident, TABLE);
- } catch (NoSuchEntityException e) {
- LOG.warn("The table to be purged does not exist in the store: {}",
ident, e);
- return false;
- } catch (Exception e) {
- throw new RuntimeException(e);
- }
-
- return isManagedEntity(catalogIdent, Capability.Scope.TABLE)
- ? droppedFromStore
- : droppedFromCatalog;
+ NameIdentifier schemaIdentifier =
NameIdentifierUtil.getSchemaIdentifier(ident);
+ return TreeLockUtils.doWithTreeLock(
+ schemaIdentifier,
+ LockType.WRITE,
+ () -> {
+ NameIdentifier catalogIdent = getCatalogIdentifier(ident);
+ boolean droppedFromCatalog =
+ doWithCatalog(
+ catalogIdent,
+ c -> c.doWithTableOps(t -> t.purgeTable(ident)),
+ RuntimeException.class,
+ UnsupportedOperationException.class);
+
+ // For unmanaged table, it could happen that the table:
+ // 1. Is not found in the catalog (dropped directly from underlying
sources)
+ // 2. Is found in the catalog but not in the store (not managed by
Gravitino)
+ // 3. Is found in the catalog and the store (managed by Gravitino)
+ // 4. Neither found in the catalog nor in the store.
+ // In all situations, we try to delete the schema from the store,
but we don't take the
+ // return value of the store operation into account. We only take
the return value of the
+ // catalog into account.
+ //
+ // For managed table, we should take the return value of the store
operation into account.
+ boolean droppedFromStore = false;
+ try {
+ droppedFromStore = store.delete(ident, TABLE);
+ } catch (NoSuchEntityException e) {
+ LOG.warn("The table to be purged does not exist in the store: {}",
ident, e);
+ return false;
+ } catch (Exception e) {
+ throw new RuntimeException(e);
+ }
+
+ return isManagedEntity(catalogIdent, Capability.Scope.TABLE)
+ ? droppedFromStore
+ : droppedFromCatalog;
+ });
}
private EntityCombinedTable importTable(NameIdentifier identifier) {
diff --git
a/core/src/main/java/org/apache/gravitino/utils/NameIdentifierUtil.java
b/core/src/main/java/org/apache/gravitino/utils/NameIdentifierUtil.java
index b656bfa95..2b7e69ebe 100644
--- a/core/src/main/java/org/apache/gravitino/utils/NameIdentifierUtil.java
+++ b/core/src/main/java/org/apache/gravitino/utils/NameIdentifierUtil.java
@@ -249,7 +249,8 @@ public class NameIdentifierUtil {
public static NameIdentifier getCatalogIdentifier(NameIdentifier ident)
throws IllegalNameIdentifierException {
NameIdentifier.check(
- ident.name() != null, "The name variable in the NameIdentifier must
have value.");
+ ident.name() != null && !ident.name().isEmpty(),
+ "The name variable in the NameIdentifier must have value.");
Namespace.check(
ident.namespace() != null && !ident.namespace().isEmpty(),
"Catalog namespace must be non-null and have 1 level, the input
namespace is %s",
@@ -265,6 +266,34 @@ public class NameIdentifierUtil {
return NameIdentifier.of(allElems.get(0), allElems.get(1));
}
+ /**
+ * Try to get the schema {@link NameIdentifier} from the given {@link
NameIdentifier}.
+ *
+ * @param ident The {@link NameIdentifier} to check.
+ * @return The schema {@link NameIdentifier}
+ * @throws IllegalNameIdentifierException If the given {@link
NameIdentifier} does not include
+ * schema name
+ */
+ public static NameIdentifier getSchemaIdentifier(NameIdentifier ident)
+ throws IllegalNameIdentifierException {
+ NameIdentifier.check(
+ ident.name() != null && !ident.name().isEmpty(),
+ "The name variable in the NameIdentifier must have value.");
+ Namespace.check(
+ ident.namespace() != null && !ident.namespace().isEmpty() &&
ident.namespace().length() > 1,
+ "Schema namespace must be non-null and at least 1 level, the input
namespace is %s",
+ ident.namespace());
+
+ List<String> allElems =
+ Stream.concat(Arrays.stream(ident.namespace().levels()),
Stream.of(ident.name()))
+ .collect(Collectors.toList());
+ if (allElems.size() < 3) {
+ throw new IllegalNameIdentifierException(
+ "Cannot create a schema NameIdentifier less than three elements.");
+ }
+ return NameIdentifier.of(allElems.get(0), allElems.get(1),
allElems.get(2));
+ }
+
/**
* Check the given {@link NameIdentifier} is a metalake identifier. Throw an
{@link
* IllegalNameIdentifierException} if it's not.
diff --git
a/server/src/main/java/org/apache/gravitino/server/web/rest/SchemaOperations.java
b/server/src/main/java/org/apache/gravitino/server/web/rest/SchemaOperations.java
index 8093da7ef..55341627b 100644
---
a/server/src/main/java/org/apache/gravitino/server/web/rest/SchemaOperations.java
+++
b/server/src/main/java/org/apache/gravitino/server/web/rest/SchemaOperations.java
@@ -210,11 +210,7 @@ public class SchemaOperations {
httpRequest,
() -> {
NameIdentifier ident = NameIdentifierUtil.ofSchema(metalake,
catalog, schema);
- boolean dropped =
- TreeLockUtils.doWithTreeLock(
- NameIdentifierUtil.ofCatalog(metalake, catalog),
- LockType.WRITE,
- () -> dispatcher.dropSchema(ident, cascade));
+ boolean dropped = dispatcher.dropSchema(ident, cascade);
if (!dropped) {
LOG.warn("Fail to drop schema {} under namespace {}", schema,
ident.namespace());
}
diff --git
a/server/src/main/java/org/apache/gravitino/server/web/rest/TableOperations.java
b/server/src/main/java/org/apache/gravitino/server/web/rest/TableOperations.java
index d5cf1ffc7..3d9d863e9 100644
---
a/server/src/main/java/org/apache/gravitino/server/web/rest/TableOperations.java
+++
b/server/src/main/java/org/apache/gravitino/server/web/rest/TableOperations.java
@@ -228,11 +228,7 @@ public class TableOperations {
httpRequest,
() -> {
NameIdentifier ident = NameIdentifierUtil.ofTable(metalake,
catalog, schema, table);
- boolean dropped =
- TreeLockUtils.doWithTreeLock(
- NameIdentifier.of(metalake, catalog, schema),
- LockType.WRITE,
- () -> purge ? dispatcher.purgeTable(ident) :
dispatcher.dropTable(ident));
+ boolean dropped = purge ? dispatcher.purgeTable(ident) :
dispatcher.dropTable(ident);
if (!dropped) {
LOG.warn("Failed to drop table {} under schema {}", table,
schema);
}
