yuqi1129 commented on code in PR #13198:
URL: https://github.com/apache/gravitino/pull/13198#discussion_r4080999579
##########
core/src/main/java/org/apache/gravitino/storage/relational/RelationalEntityStore.java:
##########
@@ -311,6 +312,25 @@ public boolean delete(NameIdentifier ident,
Entity.EntityType entityType, boolea
}
}
+ @Override
+ public EntityVersion getVersion(NameIdentifier ident, Entity.EntityType
entityType)
+ throws IOException {
+ // Always read through: a cached entity does not carry the store version,
and a stale cache
+ // entry must never be the basis of a version check.
+ return backend.getVersion(ident, entityType);
+ }
+
+ @Override
+ public boolean delete(
+ NameIdentifier ident, Entity.EntityType entityType, boolean cascade,
EntityVersion expected)
+ throws IOException {
+ try {
+ return backend.delete(ident, entityType, cascade, expected);
+ } finally {
+ cache.invalidate(ident, entityType);
Review Comment:
Fixed in 8ac260668. The version-checked delete now calls invalidateCache(),
so it advances the epoch before invalidating the entry. I added
testBatchGetSkipsWriteBackWhenVersionCheckedDeleteInvalidatesDuringBackendRead
to reproduce the late-fill interleaving. The targeted dispatcher and late-fill
suites pass (33 + 9 tests).
##########
core/src/main/java/org/apache/gravitino/catalog/OperationDispatcher.java:
##########
@@ -244,6 +249,102 @@ protected StringIdentifier
getStringIdFromProperties(Map<String, String> propert
}
}
+ /**
+ * Wraps an updater so the store rejects the update, before writing
anything, when the row under
+ * the name is not the entity the external catalog reported.
+ *
+ * <p>The updater runs inside the store's update, after the current row is
read and before the
+ * version-checked write. Throwing here therefore aborts the transaction
with nothing written,
+ * which is what a post-write id comparison cannot do.
+ *
+ * @param expectedId the id read from the external catalog
+ * @param updater the update to apply when the ids match
+ * @param <E> the entity type
+ * @return the guarded updater
+ */
+ protected static <E extends Entity & HasIdentifier> Function<E, E>
requireEntityId(
+ long expectedId, Function<E, E> updater) {
+ return entity -> {
+ if (entity.id() != expectedId) {
+ throw new EntityIdMismatchException(entity.id(), expectedId);
+ }
+ return updater.apply(entity);
+ };
+ }
+
+ /**
+ * Reads the id and store version of a registration before an
external-catalog call, so a later
+ * store write can be fenced on it.
+ *
+ * @param ident the entity identifier
+ * @param type the entity type
+ * @return the observed id and version, or null when nothing is registered
under the name or the
+ * store cannot read versions
+ */
+ @Nullable
+ protected EntityVersion observeRegistration(NameIdentifier ident,
Entity.EntityType type) {
+ try {
+ return store.getVersion(ident, type);
+ } catch (NoSuchEntityException | UnsupportedOperationException e) {
+ return null;
Review Comment:
Fixed in 8ac260668. observeRegistration now treats only
NoSuchEntityException as an absent registration. Unsupported getVersion fails
before the external drop. I also removed the broad
UnsupportedOperationException catch around the version-checked delete: if that
operation is unsupported, the store registration is left in place and the
failure is reported instead of deleting by name. Custom stores/backends used on
these paths must implement both version operations. Tests cover both
unsupported cases, including preservation of the external table when the
version read fails.
##########
core/src/main/java/org/apache/gravitino/catalog/SchemaOperationDispatcher.java:
##########
@@ -556,33 +559,31 @@ public boolean dropSchema(NameIdentifier ident, boolean
cascade) throws NonEmpty
schemaProperties = new HashMap<>(schemaEntity.properties());
}
+ // For managed schema, we don't need to drop the schema from the
store again.
+ boolean isManagedSchema = isManagedEntity(catalogIdent,
Capability.Scope.SCHEMA);
+ // Read the registration before the external call, so the store
delete below can only
+ // remove the row this drop started with and never one re-created
under the same name.
+ EntityVersion observed = isManagedSchema ? null :
observeRegistration(ident, SCHEMA);
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) {
if (droppedFromCatalog) {
secretManager.deleteSecretsFromProperties(schemaProperties);
}
return droppedFromCatalog;
}
- // A non-cascading drop preserves a missing registration because the
source schema
- // may have been renamed. An explicit cascading drop also removes
stale metadata.
+ // A non-cascading false result may mean the external schema was
renamed, so preserve
+ // its registration. An explicit cascade also removes stale
metadata, but only if the
+ // registration is still the one observed before the external call.
boolean droppedFromStore = false;
if (droppedFromCatalog || cascade) {
- try {
- droppedFromStore = store.delete(ident, SCHEMA, true);
- } 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);
- }
+ droppedFromStore = deleteObservedRegistration(ident, SCHEMA, true,
observed);
Review Comment:
Agreed. The schema fence protects the observed schema row and its identity;
it does not snapshot or fence every descendant. A child registered after the
observation can still be included in the later cascade, so I would not claim
this PR solves that race. #13172 covers the external-backed entity incarnation
ABA and wrong-id update; descendant creation during cascade is a separate issue
that needs subtree-level coordination or per-child fencing. No code change for
this question in 8ac260668.
--
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]
