Copilot commented on code in PR #12350:
URL: https://github.com/apache/gravitino/pull/12350#discussion_r3765840192
##########
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())
+ :
mapper.selectCatalogMetaByIdForShare(observedCatalogPO.getCatalogId()));
+ if (currentCatalogPO == null
+ || !Objects.equals(currentCatalogPO.getCatalogName(),
observedCatalogPO.getCatalogName())
+ || !Objects.equals(currentCatalogPO.getMetalakeId(),
observedCatalogPO.getMetalakeId())) {
+ throw new NoSuchEntityException(
+ NoSuchEntityException.NO_SUCH_ENTITY_MESSAGE,
+ Entity.EntityType.CATALOG.name().toLowerCase(),
+ observedCatalogPO.getCatalogName());
+ }
+ }
+
+ private void lockCatalogForSchemaDelete(NameIdentifier identifier, SchemaPO
observedSchemaPO) {
+ CatalogPO currentCatalogPO =
+ SessionUtils.getWithoutCommit(
+ CatalogMetaMapper.class,
+ mapper ->
mapper.selectCatalogMetaByIdForUpdate(observedSchemaPO.getCatalogId()));
Review Comment:
This delete fence detects a catalog rename/delete, but not a metalake
rename: the catalog name and `metalakeId` remain unchanged after the metalake
name changes. A delete that resolved the schema under the old path can
therefore proceed after the rename and remove the schema from the new path.
Lock and validate the metalake name before taking this catalog lock, preserving
root-to-leaf lock order.
##########
core/src/main/java/org/apache/gravitino/storage/relational/service/SchemaMetaService.java:
##########
@@ -227,20 +234,23 @@ public <E extends Entity & HasIdentifier> SchemaEntity
updateSchema(
NameIdentifierUtil.ofSchema(metalakeName, catalogName,
oldSchemaEntity.name()).toString();
boolean isRenamed = !Objects.equals(oldSchemaEntity.name(),
newEntity.name());
- AtomicInteger updateResult = new AtomicInteger(0);
try {
SessionUtils.doMultipleWithCommit(
- () ->
- updateResult.set(
- SessionUtils.getWithoutCommit(
- SchemaMetaMapper.class,
- mapper ->
- ops.updatePO(
- mapper,
-
POConverters.updateSchemaPOWithVersion(oldSchemaPO, newEntity),
- oldSchemaPO))),
() -> {
- if (isRenamed && updateResult.get() > 0) {
+ int updated =
+ SessionUtils.getWithoutCommit(
+ SchemaMetaMapper.class,
+ mapper ->
+ ops.updatePO(
+ mapper,
+
POConverters.updateSchemaPOWithVersion(oldSchemaPO, newEntity),
+ oldSchemaPO));
Review Comment:
The schema CAS can still commit after either ancestor is renamed. The schema
was loaded through the old fully qualified name, but the update locks/checks
only the schema row; catalog or metalake renames do not change its IDs or
version. Consequently a request for the old path can mutate the schema now
reachable only through the new path. Acquire and validate shared ancestor locks
in metalake-to-catalog order before this CAS.
##########
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,
Review Comment:
The create fence does not cover a concurrent metalake rename. `catalogPO` is
resolved using the old metalake name before the transaction, but this lock only
revalidates the catalog row; a metalake rename leaves that row and its
`metalakeId` unchanged, so the insert can succeed under the renamed metalake
even though the requested fully qualified parent no longer exists. Lock and
validate the metalake row by ID/name before locking the catalog (root-to-leaf
order), and add a cross-transaction rename/create regression test.
##########
core/src/main/java/org/apache/gravitino/storage/relational/service/CatalogMetaService.java:
##########
@@ -226,41 +240,37 @@ public <E extends Entity & HasIdentifier> CatalogEntity
updateCatalog(
String oldFullName =
NameIdentifierUtil.ofCatalog(metalakeName,
oldCatalogEntity.name()).toString();
- AtomicInteger updateResult = new AtomicInteger(0);
try {
SessionUtils.doMultipleWithCommit(
- () ->
- updateResult.set(
- SessionUtils.getWithoutCommit(
- CatalogMetaMapper.class,
- mapper ->
- mapper.updateCatalogMeta(
- POConverters.updateCatalogPOWithVersion(
- oldCatalogPO, newEntity,
oldCatalogPO.getMetalakeId()),
- oldCatalogPO))),
() -> {
- if (updateResult.get() > 0) {
+ int updated =
+ SessionUtils.getWithoutCommit(
+ CatalogMetaMapper.class,
+ mapper ->
+ mapper.updateCatalogMeta(
+ POConverters.updateCatalogPOWithVersion(
+ oldCatalogPO, newEntity,
oldCatalogPO.getMetalakeId()),
+ oldCatalogPO));
Review Comment:
This CAS does not fence a concurrent metalake rename. After `oldCatalogPO`
is read through the old metalake name, the metalake can be renamed on another
server without changing the catalog row or version, allowing this update to
mutate the catalog now under the new path. Take and validate a shared metalake
lock before updating the catalog so stale fully qualified identifiers report
missing instead of succeeding.
##########
core/src/main/java/org/apache/gravitino/storage/relational/service/CatalogMetaService.java:
##########
@@ -415,6 +420,72 @@ public boolean deleteCatalog(NameIdentifier identifier,
boolean cascade) {
return true;
}
+ private void deleteCatalogWithVersion(NameIdentifier identifier, CatalogPO
observedCatalogPO) {
+ int deleted =
+ SessionUtils.getWithoutCommit(
+ CatalogMetaMapper.class,
+ mapper ->
+ mapper.softDeleteCatalogMetasByCatalogId(
+ observedCatalogPO.getCatalogId(),
observedCatalogPO.getCurrentVersion()));
Review Comment:
A catalog delete loaded through the old metalake name can still succeed
after that metalake is renamed, because this CAS checks only the catalog row
and its version. The catalog's `metalakeId` is stable across the rename, so the
request deletes the catalog from the new path rather than producing the
documented missing/idempotent result. Acquire and validate the metalake row
before the catalog CAS, using root-to-leaf lock order.
--
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]
