JoegenUSTC commented on code in PR #11846:
URL: https://github.com/apache/gravitino/pull/11846#discussion_r3577162816
##########
core/src/main/java/org/apache/gravitino/catalog/CapabilityHelpers.java:
##########
@@ -47,17 +47,56 @@
import org.apache.gravitino.rel.partitions.Partition;
import org.apache.gravitino.rel.partitions.Partitions;
import org.apache.gravitino.rel.partitions.RangePartition;
+import org.apache.gravitino.utils.ThrowableFunction;
public class CapabilityHelpers {
- public static Capability getCapability(NameIdentifier ident, CatalogManager
catalogManager) {
+ /**
+ * Executes {@code fn} with the {@link Capability} of the catalog identified
by {@code ident},
+ * inside the catalog's classloader boundary. This prevents the catalog's
{@link
+ * org.apache.gravitino.utils.IsolatedClassLoader} from being closed while
capability methods are
+ * executing.
+ *
+ * <p>If the first attempt encounters a stale (already-closed) {@link
+ * CatalogManager.CatalogWrapper}, the wrapper is evicted from the cache and
the call is retried
+ * once with a freshly loaded wrapper. Retry is safe because {@code fn} is
restricted to pure
+ * normalization and never performs external I/O, so re-execution is
idempotent. The retry is
+ * triggered only on {@link CatalogManager.CatalogWrapperClosedException},
which is thrown
+ * exclusively at wrapper-entry before {@code fn} has run, preventing
accidental replay of
+ * partially-executed non-idempotent operations.
+ *
+ * <p>Callers should use this method instead of calling {@code
capabilities()} on a raw wrapper
+ * invoke {@link Capability} methods so that the classloader lifecycle is
properly bounded.
+ *
+ * @param ident any {@link NameIdentifier} that belongs to the target catalog
+ * @param catalogManager the catalog manager used to load the catalog
+ * @param fn function to execute with the catalog's {@link Capability}
+ * @param <R> return type
+ * @return the result of {@code fn}
+ */
+ public static <R> R withCapability(
+ NameIdentifier ident, CatalogManager catalogManager,
ThrowableFunction<Capability, R> fn) {
NameIdentifier catalogIdent = getCatalogIdentifier(ident);
- CatalogManager.CatalogWrapper c =
catalogManager.loadCatalogAndWrap(catalogIdent);
- try {
- return c.capabilities();
- } catch (Exception e) {
- throw new RuntimeException("Failed to get capabilities for catalog: " +
catalogIdent, e);
+ CatalogManager.CatalogWrapperClosedException lastClosed = null;
+ for (int i = 0; i < 2; i++) {
+ CatalogManager.CatalogWrapper c =
catalogManager.loadCatalogAndWrap(catalogIdent);
+ try {
+ return c.doWithCapabilityOps(fn);
Review Comment:
Great question — adding sleep/jitter is a reasonable reflex for any retry
loop. However, I don't think it's necessary here, for the following reasons:
The purpose of sleep/jitter in retry logic is typically to back off from a
temporarily unavailable external resource (e.g. a server under load, a network
partition) or to spread out retry storms when many callers fail simultaneously.
Neither condition applies here:
1. **The resource is immediately available after retry.**
`CatalogWrapperClosedException` is thrown because Caffeine evicted the wrapper
and called `close()` on it. The retry calls `loadCatalogAndWrap` again, which
uses Caffeine's atomic `cache.get(key, loader)` to synchronously construct a
fresh `CatalogWrapper` and cache it. There is no waiting for external readiness
— the new wrapper is available on the very next line.
2. **No thundering herd.** Caffeine already serializes concurrent loads for
the same key internally (similar to `ConcurrentHashMap.computeIfAbsent`), so
even if many threads hit this path simultaneously, the catalog plugin is loaded
only once. There is no fan-out amplification that jitter could prevent.
3. **`fn` is pure normalization.** Since `fn` is now restricted to CPU-only
name normalization with no I/O, the retry is safe and idempotent regardless of
how quickly it fires.
If rapid re-eviction were a concern (i.e. the newly loaded wrapper is
evicted again immediately), that would indicate a Caffeine capacity/TTL
configuration issue — sleeping a few milliseconds would not help in that case
either.
The pattern is closer to a stale-cache-entry reload than a network-level
retry. Adding a sleep would only introduce unnecessary latency on an
already-rare code path without improving correctness or reducing contention.
That said, if you see a specific scenario where sleep/jitter would make a
difference, happy to discuss further and reconsider.
--
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]
