JoegenUSTC commented on PR #11846:
URL: https://github.com/apache/gravitino/pull/11846#issuecomment-5019507772
> Although this change can solve the issue, it requires switching the class
loader multiple times for every operation request, which is not a good design.
I see two possible approaches: Set the class loader at a higher level in
NormalizeDispatcher.java and wrap the entire operation with doWith, so the
lower-level code does not need to handle class-loader switching. Let Capability
hold the class loader and manage it with reference counting. This would prevent
the class loader from being closed while it is still in use and would also
solve the issue. However, this approach requires a larger change, so I would
prefer not to use it for now.
>
> However, both approaches would require relatively large changes.
@diqiu50 Thanks for the detailed review! You're right that `createTable` is
a regression — it now makes **6 separate `withCapability` calls**, producing
**14 `setContextClassLoader` operations** compared to **4 in the original
code**. That's a genuine design issue worth fixing.
That said, the picture across all methods is mixed:
- `alterTable` actually **improved** from 6 → 4 switches (the original code
called `getCapability` twice — once explicitly and once inside
`normalizeCaseSensitive`; the Pair approach merges both into a single
`withCapability`)
- `listPartitionNames` / `listPartitions` went from 4 → 6 — a necessary +2
trade-off: the original code reused one escaped `Capability` reference for both
input and output normalization, which is exactly the unsafe pattern this PR
fixes; keeping both normalizations inside the boundary requires two separate
`withCapability` calls
- All other methods remain at 4, unchanged
**On actual overhead:** with SecurityManager disabled by default in JDK 17+
(JEP 411), each `setContextClassLoader` degrades to a single reference-field
write — estimated ~3–7 ns based on OpenJDK source analysis and standard JMH
micro-benchmark ranges. Each `classLoader()` synchronized call is an
uncontended thin-lock null-check, estimated ~5–15 ns (JEP 374 removed biased
locking in JDK 15; uncontended lightweight locking via CAS on the object
header). The total extra machine cost from the 10 additional switches in
`createTable` is therefore estimated at roughly **~100 ns** — against a Hive
Metastore RPC of **100 ms+**, a ratio of < 0.0001%. It is also worth noting
that the TCCL switch is effectively a no-op for normalization: `fn` performs
pure CPU work (string lowercasing, field mapping) using only JDK standard
library and main-classloader-loaded types, and never triggers class loading —
so the isolated classloader is never actually consulted. The design concern is
val
id; the runtime cost is not.
**`createTable` is the only real regression** and can be fixed by bundling
all 6 normalizations into a single `withCapability` call — bringing it back to
4 switches while still keeping the dispatcher RPC outside the lock (per
@yuqi1129's earlier feedback):
```java
NormalizedCreateArgs norm = withCapability(ident, catalogManager, cap ->
new NormalizedCreateArgs(
applyCapabilities(ident, Capability.Scope.TABLE, cap),
applyCapabilities(columns, cap),
applyCapabilities(partitions, cap),
applyCapabilities(distribution, cap),
applyCapabilities(sortOrders, cap),
applyCapabilities(indexes, cap)));
return dispatcher.createTable(norm.ident, norm.columns, comment, properties,
...);
```
**On Approach 1** — setting the classloader at a higher level and wrapping
the entire operation with `doWith`:
If "wrapping the entire operation" means using `doWithCapabilityOps` (or any
`synchronized` wrapper) to cover both normalization and the dispatcher RPC, it
reintroduces the serialization problem @yuqi1129 identified in the previous
review: since `doWithCapabilityOps` is `synchronized` on the `CatalogWrapper`
instance, all operations on the **same catalog** would be serialized on a
single monitor. A slow Hive Metastore call — e.g., `listPartitions` over tens
of thousands of partitions taking several seconds — would block every
concurrent `createTable`, `alterTable`, and `addPartition` on that catalog.
Combined with Caffeine's `CallerRunsPolicy`, it could also stall the eviction
thread itself.
If instead it means switching the classloader without holding any lock
(bypassing `doWithCapabilityOps`), then there is no protection against
`close()` being called concurrently — the `IsolatedClassLoader` could still be
torn down while Capability methods are executing, which is precisely the bug
this PR is fixing.
Either way, Approach 1 either trades one problem for another, or requires a
new non-locking lifecycle mechanism — which converges on Approach 2.
**On Approach 2 (reference counting):** This is architecturally the cleanest
solution — `Capability` objects could be used freely without any boundary
concern, and `close()` would only fire when the refcount reaches zero. As you
noted, it requires non-trivial changes (`retain`/`release` on
`IsolatedClassLoader`, paired lifecycle management at all call sites). Would it
make sense to file a separate issue to track it as a follow-up?
Happy to apply the `createTable` bundling fix in this PR. @yuqi1129 @diqiu50
does that direction work for both of you?
--
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]
