zhoujinsong commented on PR #4311: URL: https://github.com/apache/amoro/pull/4311#issuecomment-5249792563
## Suggestion: Consolidate thread pools by latency sensitivity Thanks for this PR! Isolating snapshot expiration planning from the global worker pool is the right direction, and the implementation cleanly follows the existing `planning`/`commit` pool pattern. While reviewing, I'd like to propose a slightly different pool partitioning strategy for discussion. The core idea: **split pools by latency sensitivity rather than by operation type**, so that latency-sensitive workloads get dedicated pools while best-effort maintenance workloads share one. ### Current state (after this PR) | Pool | Used by | |---|---| | `iceberg-planning-pool` | self-optimizing plan (manifest read) | | `iceberg-commit-pool` | self-optimizing commit `scanManifestsWith` | | `iceberg-snapshot-expiration-planning-pool` (new) | snapshot expiration `.planWith(...)` | | `iceberg-worker-pool` (Iceberg global, hardcoded) | everything else: `SnapshotProducer.writeManifests`, orphan cleaning, data expiry, dangling-delete cleaning, `IcebergTableMaintainer.newRewrite()/newScan()`, `TableFileUtil`, `ServerTableDescriptor`, ... | ### Proposed partitioning | Pool | Sensitivity | Workloads | |---|---|---| | `iceberg-planning-pool` | latency-sensitive (dedicated) | self-optimizing plan | | `iceberg-commit-pool` | latency-sensitive (dedicated) | self-optimizing commit (`scanManifestsWith` now, `writeManifestsWith` once we upgrade to an Iceberg version that exposes it — see [#16108](https://github.com/apache/iceberg/pull/16108)) | | `iceberg-maintenance-pool` | best-effort (shared) | snapshot expiration, orphan file cleaning, dangling-delete cleaning, data expiry, tag auto-creation, hive sync, and other `IcebergTableMaintainer` operations | Besides, forward-compatible with Iceberg's new `writeManifestsWith` API.** Iceberg main branch ([PR #16108](https://github.com/apache/iceberg/pull/16108), merged 2026-06-01) now exposes `writeManifestsWith(ExecutorService, int parallelism)` on `SnapshotUpdate`. Once Amoro upgrades past 1.7.2, we can pass `iceberg-commit-pool` to both `scanManifestsWith` and `writeManifestsWith` — **the pool partitioning itself doesn't need to change**. ### Suggested config surface - Keep `self-optimizing.plan-manifest-io-thread-count` and `self-optimizing.commit-manifest-io-thread-count` as-is. - Replace the newly added `expire-snapshots.plan-thread-count` with a single AMS `table-manifest-io.maintenance-thread-count` (default 10) that governs the shared `iceberg-maintenance-pool`. -- 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]
