konstantinb commented on PR #8426: URL: https://github.com/apache/hadoop/pull/8426#issuecomment-4745744957
Thanks again for the thorough review, @ajfabbri — much appreciated. > I have two requested changes to (1) add to `core-default.xml` (2) remove superfluous `volatile`. Both done — removed the `volatile` (the field is only accessed inside the `synchronized` method) and added the shared-thread-pool keys to `core-default.xml` with their defaults. Also addressed the two inline nits: `@VisibleForTesting` on `createScheduledExecutor`, and explicit size/keepalive value assertions in `TestSharedScheduledExecutor`. > Final thought / question: It would be cool to add a test which actually causes runaway thread growth with S3A FS instances, and compares it w/ and w/o shared pools. I had the same instinct, so it's in this PR rather than a follow-up. Two ITests: - `ITestS3ASharedThreadPoolDisabled` — creates 20 uncached `S3AFileSystem` instances, abandons them, then forces GC until a `WeakReference` to one clears (proving the instance was really collected, since `System.gc()` is only a hint). Without sharing, the per-client `sdk-ScheduledExecutor` pools survive collection and the thread count far exceeds a single pool. - `ITestS3ASharedThreadPoolEnabled` — same scenario with the shared pool on; the thread count stays bounded. Together they demonstrate the impact of [aws-sdk-java-v2#1690](https://github.com/aws/aws-sdk-java-v2/issues/1690) — the "leaked when the client is evicted" thread growth without the shared pool, and bounded with it. > Thinking out loud on the lifecycle and ownership question... The fact that these are opt-in configs that default to disabled mitigates a lot of the risk... Agreed. The opt-in, default-disabled design was deliberate, for exactly the risk you mention around qualifying different workloads — the change has no effect on existing deployments until someone explicitly enables it. The shared executor also uses `allowCoreThreadTimeOut(true)` with a 60s keepalive, so idle threads time out and the pool shrinks back to near zero when there's no work, instead of holding threads for the life of the JVM. As an extra check that turning it on is safe, I ran the full hadoop-aws integration suite (against LocalStack) both with all four pools enabled and with the default; the pass/fail set was identical except the disabled-control test above, which by design expects the leak — so enabling the pools didn't perturb the rest of the suite. > Another thought for potential future work: it would be nice if the pool sizes were auto-tuned. Strongly agree on the "too many knobs" point. The current defaults are at least a reasonable baseline — size 5 mirrors the SDK's own per-client default (`Executors.newScheduledThreadPool(5)`), and the 60s keepalive adds the idle-thread timeout the SDK's default executor doesn't have. If we revisit sizing, I'd lean away from a control loop and toward the self-trimming we already have: since idle threads already time out, over-sizing is nearly free while under-sizing only costs scheduling latency, so a generous cap (e.g. derived from `availableProcessors()`) that the keepalive trims back to the actual working set removes most of the need to pick an exact number — and since this pool just fires small retry/timeout/refresh tasks, even 5 is rarely the bottleneck. The higher-value first step is probably observability rather than tuning: surfacing live scheduler-thread count and scheduling latency (e.g. via `IOStatistics`) so it's visible whether a pool is ever saturated before anyon e reaches for a knob. -- 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] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
