roryqi commented on code in PR #12350:
URL: https://github.com/apache/gravitino/pull/12350#discussion_r3718744727
##########
core/src/main/java/org/apache/gravitino/storage/relational/service/SchemaMetaService.java:
##########
@@ -492,13 +471,78 @@ private List<SchemaPO> listSchemaPOs(Namespace namespace)
{
mapper -> POStorageReadRouting.listPOs(mapper, namespace, ops,
Entity.EntityType.SCHEMA));
}
+ private void fenceCatalogForSchemaCreate(CatalogPO catalogPO) {
+ int fenced =
+ SessionUtils.getWithoutCommit(
+ CatalogMetaMapper.class,
+ mapper ->
+ mapper.fenceCatalogMeta(catalogPO.getCatalogId(),
catalogPO.getCurrentVersion()));
Review Comment:
The parent CAS means two overlapping schema creates that both read catalog
version N cannot reach the insert sequentially: the winner advances the catalog
to N+1, while the loser fails here with OptimisticLockException. For same-name
managed creates, that contradicts the stated behavior that the losing create
returns SchemaAlreadyExistsException; the current duplicate test is sequential
and does not exercise this interleaving. It also makes concurrent creates of
unrelated sibling schemas conflict. Could we either use a parent row
lock/revalidation strategy or explicitly retry the fence while the parent
remains active, and add a true concurrent same-name create test?
##########
core/src/main/java/org/apache/gravitino/storage/relational/service/MetalakeMetaService.java:
##########
@@ -206,30 +210,32 @@ public <E extends Entity & HasIdentifier> BaseMetalake
updateMetalake(
throw re;
}
- if (updateResult.get() > 0) {
- return newMetalakeEntity;
- } else {
- throw new IOException("Failed to update the entity: " + ident);
- }
+ return newMetalakeEntity;
}
@Monitored(
metricsSource = GRAVITINO_RELATIONAL_STORE_METRIC_NAME,
baseMetricName = "deleteMetalake")
public boolean deleteMetalake(NameIdentifier ident, boolean cascade) {
NameIdentifierUtil.checkMetalake(ident);
- Long metalakeId = getMetalakeIdByName(ident.name());
+ MetalakePO metalakePO =
+ SessionUtils.getWithoutCommit(
+ MetalakeMetaMapper.class, mapper ->
mapper.selectMetalakeMetaByName(ident.name()));
+ if (metalakePO == null) {
+ throw new NoSuchEntityException(
+ NoSuchEntityException.NO_SUCH_ENTITY_MESSAGE,
+ Entity.EntityType.METALAKE.name().toLowerCase(),
+ ident.toString());
+ }
+ Long metalakeId = metalakePO.getMetalakeId();
+ Long currentVersion = metalakePO.getCurrentVersion();
if (metalakeId != null) {
if (cascade) {
SessionUtils.doMultipleWithCommit(
- () ->
- SessionUtils.doWithoutCommit(
- MetalakeMetaMapper.class,
- mapper ->
mapper.softDeleteMetalakeMetaByMetalakeId(metalakeId)),
- () ->
- SessionUtils.doWithoutCommit(
- CatalogMetaMapper.class,
- mapper ->
mapper.softDeleteCatalogMetasByMetalakeId(metalakeId)),
+ () -> {
+ deleteMetalakeWithVersion(ident, metalakeId, currentVersion);
+ deleteCatalogsWithVersions(ident, metalakeId);
Review Comment:
**Blocking:** This cascade path CAS-deletes catalogs, but the immediately
following schema cleanup still soft-deletes schemas only by metalake ID,
without checking their observed OCC versions. A concurrent schema alter does
not fence the metalake or catalog, so it can commit a newer schema version
before that statement; the unconditional cleanup will then delete the newer row
and the metalake deletion succeeds instead of reporting an
OptimisticLockException. This also contradicts the PR description that
descendant schemas and catalogs are CAS-deleted with identifier-and-version
pairs. Could we collect the SchemaPOs and batch-delete them by ID/version,
verify the affected-row count, and add an interleaving regression test?
--
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]
