roryqi commented on code in PR #12404:
URL: https://github.com/apache/gravitino/pull/12404#discussion_r3931644298
##########
core/src/main/java/org/apache/gravitino/catalog/OperationDispatcher.java:
##########
@@ -105,8 +106,9 @@ protected <R, E extends Throwable> R doWithCatalog(
NameIdentifier ident, ThrowableFunction<CatalogManager.CatalogWrapper,
R> fn, Class<E> ex)
throws E {
try {
- CatalogManager.CatalogWrapper c =
catalogManager.loadCatalogAndWrap(ident);
- return fn.apply(c);
+ try (CatalogLease lease = catalogManager.acquireCatalogLease(ident)) {
+ return fn.apply(lease.wrapper());
Review Comment:
The lease ends as soon as fn.apply returns, but R may still be a live
connector object that the caller continues to access. For example,
TableOperationDispatcher.alterTable calls alteredTable.properties() after this
method has closed the lease, and internalLoadTable subsequently acquires
separate leases for capability/property metadata. A concurrent invalidation can
therefore retire the old wrapper before the returned object is fully consumed,
preserving the original use-after-retirement window and potentially mixing an
object from the old wrapper with metadata from a newly loaded one. Please
either keep one lease for the complete top-level operation or eagerly snapshot
all connector-backed data before closing it. A single operation-scoped lease
would also remove several repeated acquisitions.
##########
core/src/main/java/org/apache/gravitino/catalog/CatalogManager.java:
##########
@@ -1015,9 +1240,12 @@ public Catalog alterCatalog(NameIdentifier ident,
CatalogChange... changes)
CatalogEntity convertedCatalog =
convertFilesetCatalogEntity(updatedCatalog);
// Use put() instead of get() to force the updated wrapper into
the cache, preventing
// a background thread from overwriting it with stale data between
invalidate and put.
- CatalogWrapper newWrapper = createCatalogWrapper(convertedCatalog,
null);
- catalogCache.put(convertedCatalog.nameIdentifier(), newWrapper);
- return newWrapper.catalog();
+ try (CatalogLease lease =
+ createAndCacheCatalogLease(
Review Comment:
The catalog mutation is committed by alterCatalogUnderLock before
createAndCacheCatalogLease acquires the lifecycle read lock. If close() obtains
the write lock in that gap, checkOpen() fails here after the entity and any
secret changes have already been persisted, so the caller sees a failed alter
even though the change took effect. Please hold the lifecycle read lock from
before alterCatalogUnderLock through cache publication (using the same lock
ordering as this callback), and add a deterministic close-vs-alter test for
this window.
--
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]
