[KYUUBI #7662][SERVER] Extend virtual thread support to server executors - #7663
[KYUUBI #7662][SERVER] Extend virtual thread support to server executors#7663wangzhigang1999 wants to merge 5 commits into
Conversation
8cc4d7a to
f40bf77
Compare
f40bf77 to
1ebcdd8
Compare
|
Hi @pan3793, this PR has changed quite a bit since your earlier review. I adopted your suggested The scope is now stable. All virtual-thread options remain disabled by default, and explicit component-level settings take precedence over the umbrella switch. Could you take another look when you have time? Thanks! |
| "Kyuubi server. This requires Java 21 or later. Enabling this option is recommended for " + | ||
| "I/O-bound Kyuubi Server deployments on Java 25 or later. On Java 21, enable virtual " + | ||
| "threads selectively after validating synchronized blocking paths for carrier-thread " + | ||
| "pinning. An explicitly configured component-level virtual thread option takes precedence.") |
There was a problem hiding this comment.
shorten the words for the Java version requirement to:
Requires at least Java 21; Java 25 or later is recommended.
also apply this to all component-level configs docs
pan3793
left a comment
There was a problem hiding this comment.
Thanks for the PR. The overall shape looks good: opt-in global switch via fallbackConf, audience(SERVER) + immutable on every entry, engine side untouched, and solid test coverage including the JDK < 21 negative path. A few comments:
1. Misconfiguration surfaces too late and too obscurely on JDK < 21
Only SessionManager.initialize and the binary frontend fail at startup; the other converted paths fail lazily (ProcBuilder's lazy val factory at first engine launch, KyuubiSyncThriftClient at first session RPC, MetadataManager / DataAgentResource at first use), with a reflective IllegalStateException deep in the stack.
Suggest failing fast at the config layer: keep a list of all VT entries in object KyuubiConf right after their definitions (essentially the serverVirtualThreadConfigs list that KyuubiConfSuite already builds — move it to main code), and add a check method in case class KyuubiConf that reads each entry and throws if any resolves to true while !ThreadUtils.isVirtualThreadSupported, naming the offending key. Invoke it once at server startup, after the conf is fully loaded — note it cannot run at KyuubiConf construction time because loadFileDefaults() populates values after the constructor. This turns every lazy runtime failure into one clear boot-time error, and the list keeps future VT entries from being forgotten.
2. Virtual threads lose the Kyuubi uncaught exception handler
NamedThreadFactory installs kyuubiUncaughtExceptionHandler; ThreadUtils.newVirtualThreadFactory does not. For executor-managed tasks this rarely matters (FutureTask captures exceptions), but ProcBuilder starts a raw thread from the factory, so the log-capture path regresses from Kyuubi-logged to stderr-only uncaught errors. Thread.Builder.uncaughtExceptionHandler(...) exists on JDK 21 and can be wired through the same reflection.
3. getPoolSize double-counts against the queue gauge
BoundedQueuedExecutorService.getPoolSize returns activeCount + queueSize, while KyuubiSessionManager registers EXEC_POOL_ALIVE and EXEC_POOL_WORK_QUEUE_SIZE as separate gauges (also exposed via SessionsResource). With platform pools, pool size excludes queued tasks, so queued tasks now show up in both gauges. Since there is no idle-worker concept in the VT model, getPoolSize = getActiveCount would keep dashboards comparable.
4. FIFO ordering is no longer guaranteed — worth a code comment
The fair runningTasks semaphore orders by arrival at acquire(), and VT start order is not guaranteed to match submission order. Current call sites are safe (I checked withLockAcquiredAsyncRequest: it submits and blocks on task.get() while holding lock, so at most one task is ever in flight), but a short comment on BoundedQueuedExecutorService would prevent a future call site from relying on FIFO. Related nit: given that serialization, the Int.MaxValue - 1 queue for asyncRequestExecutor can never hold more than one task; a smaller value or a comment would state intent better.
5. Component docs omit the inheritance
The generated settings.md renders a literal false default for every component entry, and none of the .doc() texts mention they inherit kyuubi.server.virtualThreads.enabled. Adding "Defaults to kyuubi.server.virtualThreads.enabled." to each component doc would make the rendered docs self-contained.
Why are the changes needed?
PR #7656 added virtual-thread support to the Binary frontend. This follow-up extends the same optional model to other I/O-bound Kyuubi Server executors and adds one global opt-in switch with component-level overrides.
The feature remains disabled by default. Engine-side executors are unchanged. See #7662 for the scope and benchmark results.
How was this patch tested?
dev/reformatdev/gen/gen_all_config_docs.shgit diff --checkWas this patch assisted by generative AI tooling?
Assisted-by: OpenAI Codex (GPT-5)