reiabreu commented on PR #9097:
URL: https://github.com/apache/storm/pull/9097#issuecomment-5919363918
*This comment was drafted with the help of an LLM (Claude) and reviewed by
me before posting.*
Reviewed this in detail — two real behavioral side effects worth addressing
before merge, plus some smaller items. Ordered by importance:
**1. DRPC_INVOCATIONS concurrency silently jumps 5→64 when the flag is
enabled**
`DRPC_INVOCATIONS` has no queue-size config (hardcoded `null` in
`ThriftConnectionType`), so the new guard `if (queueSize != null ||
StormThreadFactory.isVirtualEnabled(topoConf))` is always true for this type
once the flag is on. That forces an explicit `ThreadPoolExecutor(64, 64, ...)`
where before it fell back to `THsHaServer`'s own pool, capped at 5 by
`corePoolSize` (unbounded queue means `maxWorkerThreads` never actually kicks
in). For DRPC_INVOCATIONS this isn't an edge case — it's a permanent,
unconditional 13x concurrency jump, not just a thread-implementation swap.
Worth documenting explicitly, or capping the new executor at the old effective
concurrency for connection types with no queue-size config.
**2. Worker shared-executor pool size becomes topology-overridable**
```java
// before: bare `conf` in this instance method resolves to this.conf
(daemon-level config)
int threadPoolSize =
ObjectReader.getInt(conf.get(Config.TOPOLOGY_WORKER_SHARED_THREAD_POOL_SIZE));
// after: explicitly passes this.topologyConf (merged: daemon conf +
topology's own overrides)
return ImmutableMap.of(WorkerTopologyContext.SHARED_EXECUTOR,
makeSharedExecutor(topologyConf));
```
Pre-PR, `topology.worker.shared.thread.pool.size` only ever read from the
daemon/cluster conf. Post-PR it reads from the merged topology conf, silently
letting a topology submitter override a pool size an operator may be relying on
as a cluster-wide cap (`TestUtilsForWorkerState.java` needed a new entry added
to its `topologyConf` map, or the test throws — confirms the read source really
moved). Probably fine as a side effect of making the flag itself
topology-overridable (which *is* documented), but worth confirming that's
deliberate — it's an undocumented trust-boundary change on multi-tenant
clusters.
**3. Virtual threads are always daemon threads, with no override**
`StormThreadFactory`'s platform branch explicitly sets `.daemon(false)`; the
virtual branch can't — `Thread.Builder.OfVirtual` has no `.daemon(...)` method,
so virtual threads are always daemon. This is a real asymmetry across all 7
pools this PR touches, but since Storm daemons shut down via explicit
`Utils.exitProcess()`/`System.exit()` calls rather than relying on natural-exit
semantics, the practical impact may be minimal — `System.exit()` doesn't wait
for non-daemon threads either, unless a shutdown hook explicitly joins them.
Worth a quick check for any `addShutdownHook` usage around these pools before
dismissing it, and a one-line doc-comment note either way.
**4. Benchmark's proxy handler misroutes `Object` methods**
(`ThriftHandlerVirtualThreadsBench.sleepingHandler`)
```java
if (method.getDeclaringClass() == Object.class) {
return method.invoke(inFlight, margs); // should be `proxy`, not
`inFlight`
}
```
`equals`/`hashCode`/`toString` get invoked on the captured `AtomicInteger`
counter instead of the proxy. Harmless today (nothing calls these on the
proxy), confined to the new example/benchmark file — just a copy-paste slip
worth a one-line fix.
**5. `AsyncLocalizer` thread names changed format**
`"AsyncLocalizer Download Executor - 0"` →
`"AsyncLocalizer-Download-Executor-0"`. No functional effect found in-repo, but
worth a heads-up for anyone with external log/metrics tooling matching the old
literal name.
**Smaller cleanup, non-blocking:**
- `StormThreadFactory.isVirtualEnabled()` writes its own boolean-parsing
logic instead of using `ObjectReader.getBoolean`, which is already used for
every other `@IsBoolean` config key in this codebase.
- `type.name().toLowerCase(Locale.ROOT) + "-handler"` is copy-pasted
identically across
`SimpleTransportPlugin`/`SaslTransportPlugin`/`TlsTransportPlugin` — could be
one method on `ThriftConnectionType`, consistent with how every other per-type
value is already sourced.
- `AssignmentDistributionService`'s new `@SuppressWarnings("unchecked")`
cast could be avoided by having `StormThreadFactory` accept a raw `Map`.
- The four new virtual-thread tests in `AuthTest.java` are near-identical
copy-paste — could collapse into one `@ParameterizedTest`.
--
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]
