yuqi1129 commented on code in PR #12350:
URL: https://github.com/apache/gravitino/pull/12350#discussion_r3766758950
##########
core/src/main/java/org/apache/gravitino/storage/relational/service/SchemaMetaService.java:
##########
@@ -492,13 +470,151 @@ private List<SchemaPO> listSchemaPOs(Namespace
namespace) {
mapper -> POStorageReadRouting.listPOs(mapper, namespace, ops,
Entity.EntityType.SCHEMA));
}
+ private void lockCatalogForSchemaCreate(
+ CatalogPO observedCatalogPO, boolean createsImplicitAncestors) {
+ CatalogPO currentCatalogPO =
+ SessionUtils.getWithoutCommit(
+ CatalogMetaMapper.class,
+ mapper ->
+ createsImplicitAncestors
+ ?
mapper.selectCatalogMetaByIdForUpdate(observedCatalogPO.getCatalogId())
Review Comment:
You are right. When a create needs to make implicit ancestors, we take
an exclusive lock on the catalog row, so all other schema creates under
the same catalog have to wait, even if they use a different path.
We need the exclusive lock here because two concurrent creates can both
see that an ancestor is missing and both insert it. Under MySQL
REPEATABLE READ, a shared lock is not enough to stop this.
So this is an accepted tradeoff: correctness first, less concurrency for
hierarchical creates. I will add this to the PR description.
If it becomes a problem later, we can make the lock smaller: only lock
the ancestor rows we really need to create, and use the unique
constraint plus a retry instead of locking the whole catalog row. I can
do that in a follow-up issue.
##########
core/src/main/java/org/apache/gravitino/metalake/MetalakeManager.java:
##########
@@ -358,7 +358,9 @@ public boolean dropMetalake(NameIdentifier ident, boolean
force)
}
return store.delete(ident, EntityType.METALAKE, true);
- } catch (NoSuchMetalakeException e) {
+ } catch (NoSuchMetalakeException | NoSuchEntityException e) {
Review Comment:
This is intentional, not a gap. There are two reasons.
First, `catalogCache` is not an entity cache. It caches `CatalogWrapper`
objects, which hold the live catalog instance and its class loader. A
metalake has nothing like this, and `MetalakeManager` has no cache field
at all. The "preload all metalakes" comment in the constructor talks
about the entity cache in the store, not a cache in the manager.
Second, the entity cache in the store is already invalidated for us.
`RelationalEntityStore.delete()` calls `cache.invalidate(ident, entityType)`
in a `finally` block, so it runs both when the delete succeeds and when
the entity is already gone. So there is no stale entry left for this
branch to clean up.
Also, in this method the `NoSuchMetalakeException` / `NoSuchEntityException`
mostly comes from `metalakeInUse()` and `store.list()`, which run before
we reach the delete.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
