Skip to content

[AMORO-4264][AMS] Use a dedicated thread pool for snapshot expiration planning - #4311

Open
johntomcat7408-cmyk wants to merge 5 commits into
apache:masterfrom
johntomcat7408-cmyk:fix-4264-expire-snapshots-thread-pool
Open

[AMORO-4264][AMS] Use a dedicated thread pool for snapshot expiration planning#4311
johntomcat7408-cmyk wants to merge 5 commits into
apache:masterfrom
johntomcat7408-cmyk:fix-4264-expire-snapshots-thread-pool

Conversation

@johntomcat7408-cmyk

Copy link
Copy Markdown
Contributor

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?

  • Add test cases that check positive and negative cases.
  • Screenshots are not applicable.
  • Run tests locally and on a Linux VM.

Validated on Linux with both JDK 11 and JDK 17:

./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.

@zhoujinsong

Copy link
Copy Markdown
Contributor

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)
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, 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 writeManifestsWiththe 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.

@johntomcat7408-cmyk

Copy link
Copy Markdown
Contributor Author

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Improvement]: use seperate threadpool with table.expireSnapshots()

2 participants