You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@amoro.apache.org by "johntomcat7408-cmyk (via GitHub)" <gi...@apache.org> on 2026/08/08 08:47:20 UTC
[PR] [AMORO-4264][AMS] Use a dedicated thread pool for snapshot expiration planning [amoro]
johntomcat7408-cmyk opened a new pull request, #4311:
URL: https://github.com/apache/amoro/pull/4311
## Why are the changes needed?
Snapshot expiration planning uses Iceberg's global worker pool by default. Under load, expiration planning can occupy that shared pool while self-optimizing commits wait for worker futures, leaving tables in the committing state for a long time.
Close #4264.
## Brief change log
- Add a process-wide planning pool dedicated to snapshot expiration.
- Configure its size with `expire-snapshots.plan-thread-count` (default: `10`) from the Iceberg process factory.
- Pass the dedicated executor to `ExpireSnapshots.planWith(...)`.
- Add regression coverage for pool isolation, configuration validation, and the expiration call path.
## How was this patch tested?
- [x] Add test cases that check positive and negative cases.
- [x] Screenshots are not applicable.
- [x] Run tests locally and on a Linux VM.
Validated on Linux with both JDK 11 and JDK 17:
```bash
./mvnw test \
-pl amoro-ams \
-am \
-Pskip-dashboard-build \
-Dtest=TestIcebergThreadPools,TestIcebergTableMaintainer,TestIcebergProcessFactory,TestSnapshotExpireIceberg \
-Dsurefire.failIfNoSpecifiedTests=false
```
Both runs completed with `BUILD SUCCESS`. The focused unit tests passed without failures or errors, and `TestSnapshotExpireIceberg` reported 0 failures and 0 errors.
## Documentation
- Does this pull request introduce a new feature? No; it isolates an existing maintenance operation and adds a tuning option.
- The default option is included in `process-factories.yaml`, and the pool scope and fallback behavior are documented in JavaDocs.
--
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: commits-unsubscribe@amoro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [AMORO-4264][AMS] Use a dedicated thread pool for snapshot expiration planning [amoro]
Posted by "zhoujinsong (via GitHub)" <gi...@apache.org>.
zhoujinsong commented on PR #4311:
URL: https://github.com/apache/amoro/pull/4311#issuecomment-5311361787
@johntomcat7408-cmyk
I'm in favor of the narrower change — generalizing the pool and using it only for snapshot expiration in this PR. We can incrementally migrate the other maintenance operations in follow-up PRs.
--
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: commits-unsubscribe@amoro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [AMORO-4264][AMS] Use a dedicated thread pool for snapshot expiration planning [amoro]
Posted by "zhoujinsong (via GitHub)" <gi...@apache.org>.
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: commits-unsubscribe@amoro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [AMORO-4264][AMS] Use a shared pool for Iceberg maintenance planning [amoro]
Posted by "zhoujinsong (via GitHub)" <gi...@apache.org>.
zhoujinsong merged PR #4311:
URL: https://github.com/apache/amoro/pull/4311
--
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: commits-unsubscribe@amoro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [AMORO-4264][AMS] Use a dedicated thread pool for snapshot expiration planning [amoro]
Posted by "johntomcat7408-cmyk (via GitHub)" <gi...@apache.org>.
johntomcat7408-cmyk commented on PR #4311:
URL: https://github.com/apache/amoro/pull/4311#issuecomment-5252927802
Thanks for the suggestion. I agree that partitioning the pools by latency sensitivity is a better long-term abstraction and avoids adding a dedicated pool and configuration for each maintenance operation. My proposed interpretation is to generalize the new pool and configuration to iceberg-maintenance-pool and table-manifest-io.maintenance-thread-count, use snapshot expiration as the first consumer, and route the other maintenance paths that currently expose an executor hook to the shared pool. Amoro currently uses Iceberg 1.7.2, where not every listed maintenance operation exposes such a hook, so those operations would remain unchanged until the Iceberg upgrade. Please let me know whether you prefer this scope, or a narrower change in this PR that only generalizes the pool and uses it for snapshot expiration.
--
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: commits-unsubscribe@amoro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org