[#12403] fix(core): defer catalog wrapper cleanup with an operation lease - #12404
Open
yuqi1129 wants to merge 7 commits into
Open
[#12403] fix(core): defer catalog wrapper cleanup with an operation lease#12404yuqi1129 wants to merge 7 commits into
yuqi1129 wants to merge 7 commits into
Conversation
…ions CatalogManager closed CatalogWrapper synchronously from the catalog cache removal listener. Caffeine runs that listener asynchronously and outside the local TreeLock, so an expiry, a remote change-log invalidation or a drop could close the catalog, clear its reference and release the pooled ClassLoader while another thread was still running an operation on that wrapper. CatalogWrapper now counts active operations: tryAcquire() takes a lease, release() returns it and retire() (called from the removal listener) only marks the wrapper unusable for new leases. The catalog and the ClassLoader are cleaned up exactly once, when the wrapper is retired and its last lease is released. Operations obtain a CatalogLease from CatalogManager.acquireCatalogLease(), which reloads a fresh wrapper when the cached one has already retired, and all production uses of the wrapper were migrated to it.
Contributor
There was a problem hiding this comment.
Pull request overview
Warning
Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.
This PR introduces an operation-lease mechanism around cached CatalogWrapper instances to prevent cache eviction/removal listeners from tearing down catalogs/classloaders while operations are still in-flight.
Changes:
- Added
CatalogLeaseandCatalogManager#acquireCatalogLease(...)to keep wrappers alive during operations and reload retired wrappers with bounded retries. - Updated production callers to use try-with-resources leasing instead of directly using
loadCatalogAndWrap(...). - Expanded unit test coverage to validate deferred cleanup, retired-wrapper reload, and concurrency scenarios.
Reviewed changes
Copilot reviewed 17 out of 17 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| iceberg/iceberg-rest-server/src/test/java/org/apache/gravitino/iceberg/service/dispatcher/TestIcebergAsyncPurge.java | Mocks the new lease acquisition path in REST server tests |
| iceberg/iceberg-rest-server/src/main/java/org/apache/gravitino/iceberg/service/dispatcher/IcebergCleanupHelper.java | Uses CatalogLease to safely access catalog ID |
| core/src/test/java/org/apache/gravitino/hook/TestTopicHookDispatcher.java | Updates mocks to support leasing |
| core/src/test/java/org/apache/gravitino/hook/TestTableHookDispatcher.java | Updates mocks to support leasing |
| core/src/test/java/org/apache/gravitino/hook/TestSchemaHookDispatcher.java | Updates mocks to support leasing |
| core/src/test/java/org/apache/gravitino/hook/TestModelHookDispatcher.java | Updates mocks to support leasing |
| core/src/test/java/org/apache/gravitino/hook/TestFunctionHookDispatcher.java | Updates mocks to support leasing |
| core/src/test/java/org/apache/gravitino/hook/TestFilesetHookDispatcher.java | Updates mocks to support leasing |
| core/src/test/java/org/apache/gravitino/catalog/TestTableNormalizeDispatcher.java | Updates mocks to support leasing |
| core/src/test/java/org/apache/gravitino/catalog/TestPartitionNormalizeDispatcher.java | Updates mocks to support leasing |
| core/src/test/java/org/apache/gravitino/catalog/TestFunctionOperationDispatcher.java | Updates mocks to support leasing for multiple catalogs |
| core/src/test/java/org/apache/gravitino/catalog/TestCatalogWrapperLease.java | New tests validating deferred cleanup and concurrent eviction safety |
| core/src/test/java/org/apache/gravitino/catalog/TestCatalogManager.java | Adapts tests to wrapper retirement / tryAcquire semantics |
| core/src/main/java/org/apache/gravitino/catalog/OperationDispatcher.java | Uses leases for catalog/table operations to avoid use-after-eviction |
| core/src/main/java/org/apache/gravitino/catalog/CatalogManager.java | Implements wrapper lease counting, retirement, and deferred cleanup |
| core/src/main/java/org/apache/gravitino/catalog/CatalogLease.java | New AutoCloseable lease implementation |
| core/src/main/java/org/apache/gravitino/catalog/CapabilityHelpers.java | Uses leases when fetching capabilities |
Suppressed comments (1)
core/src/main/java/org/apache/gravitino/catalog/CatalogManager.java:1
catalog()readscatalogwithout synchronization/volatile, whilecleanup()mutatescatalog(andpoolEntry) outside ofleaseLock. This is a Java memory-model data race and can lead to visibility issues or observing partially-updated state across threads. A concrete fix is to either (a) make the mutated/read fields (catalog, and potentiallypoolEntry/classLoader)volatile, and/or (b) move the state-nullification (catalog = null,poolEntry = null, etc.) into asynchronized (leaseLock)block after closing resources so publication is properly ordered.
/*
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
…vate Drop CatalogLease.of(), which existed only so tests with a mocked CatalogManager could hand out a lease, and narrow tryAcquire/release/retire/ isRetired to package-private: every production caller lives in the catalog package. Tests outside that package now build leases through a test-only CatalogTestUtils helper. Also document that the deferred cleanup runs on the thread that releases the last lease, which is a request thread when the wrapper was evicted while an operation was in flight.
…tCapability CapabilityHelpers.getCapability() acquired the catalog lease inside the try block that wraps failures into a RuntimeException, so a missing catalog turned a 404 into a 500 on the normalize paths. Acquire the lease outside the try, as the catalog load was before, and wrap only the capability lookup.
- Make CatalogWrapper#catalog volatile so the cleanup that nulls it outside leaseLock is properly published to unleased readers (loadCatalogAndWrap callers); keep the nulling out of the lock so a slow catalog close cannot stall tryAcquire. Make classLoader and pool final, they never change after construction. - Keep the Javadoc @link to acquireCatalogLease on one line by importing NameIdentifier, so the link renders. - Build the expiring test's SecretManager from the same config as its CatalogManager.
Code Coverage Report
Files
|
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 19 out of 19 changed files in this pull request and generated no new comments.
Suppressed comments (1)
core/src/main/java/org/apache/gravitino/catalog/CatalogManager.java:425
- cleanup() can leave
catalognon-null ifcatalog.close()throws, because the assignmentcatalog = nullis after the close call inside the lambda. SincecleanupStartedis then set, cleanup won’t be retried, leaving a stale reference (and potentially resources) around even though the wrapper is retired.
classLoader.withClassLoader(
cl -> {
if (catalog != null) {
catalog.close();
}
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What changes were proposed in this pull request?
CatalogManager.CatalogWrappernow counts active operations:tryAcquire()takes a lease,release()returns it, andretire()marks the wrapper unusable for new leases without tearing anything down. The catalog and theIsolatedClassLoaderare cleaned up exactly once, when the wrapper is retired and its last lease has been released (cleanup runs outside the lock so a slow catalog close cannot block new lease attempts).retire()instead ofclose();close()is kept as an alias ofretire()for exclusive owners such astestConnection.CatalogLease(AutoCloseable) obtained fromCatalogManager.acquireCatalogLease(ident), which reloads a fresh wrapper (bounded retries) when the cached one has already retired. The lease methods on the wrapper stay package-private; every production caller lives in thecatalogpackage.loadCatalog,enableCatalog,disableCatalog,alterCatalog,dropCatalog,getResolvedProperties,OperationDispatcher.doWithTable/doWithCatalog,CapabilityHelpers.getCapabilityandIcebergCleanupHelper.catalogId.loadCatalogAndWrapstays for compatibility (documented as unleased) and now detects a stale cache entry viaisRetired()instead ofcatalog() != null.Why are the changes needed?
CatalogWrapper.close()was invoked from the cache removal listener, which Caffeine runs asynchronously and outside the localTreeLock. A cache expiry, a remote change-log invalidation, or a drop could therefore close the catalog, null its reference and release the pooled ClassLoader while another thread was still using the wrapper, surfacing asNullPointerException,NoClassDefFoundError, or errors from prematurely closed catalog resources.Fix: #12403
One behaviour note for reviewers: when a wrapper is evicted while an operation is in flight, the deferred cleanup runs on the thread that releases the last lease, i.e. a request thread, so a slow catalog close is charged to that request. In the common case (no lease at eviction time) cleanup still runs on the cache's own thread. Offloading the deferred cleanup to an executor is a possible follow-up; it was left out here to keep the change small.
Does this PR introduce any user-facing change?
No. New internal APIs only (
CatalogLease, and package-private lease methods onCatalogWrapper); no configuration or REST API change.How was this patch tested?
TestCatalogWrapperLease(8 tests): cache expiry, explicit invalidation, remote change-log invalidation, drop, retired-wrapper reload, exactly-once ClassLoader cleanup (verified through the shared pool reference count), release-without-acquire, and a two-thread repro where the cache is invalidated while an operation holds the wrapper../gradlew :core:test :server:test :iceberg:iceberg-rest-server:test -PskipITsall pass (core: 1618 tests); repo-widecompileJava/compileTestJava,spotlessCheckandjavadocare clean.