feat(iceberg): add commit retry with exponential backoff on 409 conflict by burnerlee · Pull Request #1206 · timeplus-io/proton

burnerlee · 2026-07-04T14:17:29Z

PR checklist:

Did you run ClangFormat?
Did you separate headers to a different section in existing community code base?
Did you surround proton: starts/ends for new code in existing community code base?

Summary

When a REST catalog (e.g. AWS Glue) runs a compaction job concurrently with the Iceberg external stream sink, commitTable returns HTTP 409 because the catalog's AssertRefSnapshotID requirement fails. This PR adds Java Iceberg-compatible commit retry with exponential backoff and includes integration tests verified against a real MinIO + Iceberg REST catalog stack.

Core changes (commit retry)

HTTP 409 from commitTable throws ICEBERG_CONFLICT_ERROR. The sink catches this, refreshes table metadata and manifest list from the catalog, and retries with a new manifest-list file (attempts counter in filename, matching Java Iceberg's SnapshotProducer convention).
Exponential backoff with four configurable session-level settings (matching Java Iceberg's commit.retry.* table properties):
- iceberg_commit_retry_num_retries (default: 20)
- iceberg_commit_retry_min_wait_ms (default: 10 000)
- iceberg_commit_retry_max_wait_ms (default: 60 000)
- iceberg_commit_retry_total_timeout_ms (default: 1 800 000)
Parquet data file and manifest are written once and reused across retries. Only the manifest list is regenerated per attempt.
Iceberg::fetchManifestList hoisted to public static for use during conflict recovery.

Bug fixes (found while writing tests)

DatabaseIceberg::createTable path-style — MinIO uses http://host:port/bucket/path not AWS virtual-hosted Bucket.s3.Region.amazonaws.com. Fix detects path-style URLs and extracts bucket correctly.
Iceberg region propagation — region setting from ExternalStreamSettings was never forwarded to S3 auth_settings, causing AWS SDK to ignore the custom endpoint override. Now propagated.
getObjectInfo no-body 404 — AWS CRT S3 client returns "No response body" for MinIO's body-less HTTP 404 on HeadObject. objectExists now treats this as not-found.

Integration tests

Four tests in tests/integration/test_iceberg_sink/ verified against MinIO + Iceberg REST catalog:

Basic write — INSERT succeeds, data readable
Retry settings accepted — non-default knobs don't block commit
Multiple batches — sequential INSERTs produce correct row count
Conflict retry — injected 409 triggers retry path, commit succeeds

Test infra additions: with_iceberg_rest cluster support, docker-compose v2 fixes, Proton-specific config overrides, url_scheme_mappers for s3://→MinIO routing.

Fixes #1084

When a REST catalog (e.g. Glue) compacts an Iceberg table concurrently with the sink, commitTable returns HTTP 409 due to optimistic-concurrency conflict on the AssertRefSnapshotID requirement. Previously Proton threw immediately with no retry, causing data loss on every compaction window. Changes: - Add ICEBERG_CONFLICT_ERROR error code (737) for HTTP 409 from commitTable - RestCatalog::commitTable throws ICEBERG_CONFLICT_ERROR on 409, keeping ICEBERG_CATALOG_ERROR for other non-2xx failures - IcebergSink::commit() wraps the manifest-list generation + commitTable in a retry loop: on conflict, refreshes table metadata and manifest list from the catalog (so parent_snapshot_id and sequence_number are current), increments attempts (new manifest-list filename per Java convention), and retries with exponential backoff - Iceberg::fetchManifestList hoisted to a public static method so the sink can call it during conflict recovery without holding an Iceberg reference - IcebergCommitRetryPolicy struct carries the four retry knobs; wired from four new global settings (matching Java Iceberg commit.retry.* semantics): iceberg_commit_retry_num_retries (default 20) iceberg_commit_retry_min_wait_ms (default 10000) iceberg_commit_retry_max_wait_ms (default 60000) iceberg_commit_retry_total_timeout_ms (default 1800000) Fixes: timeplus-io#1084

burnerlee · 2026-07-04T14:23:40Z

hey @yuzifeng1984
I have raised a PR for #1084. I am still figuring out the testing part - meanwhile, could you take a look at the approach.
I had 2 questions:

should I move the backoff retry logic to a common package?
do we want to implement retries for all sinks forcing all to implement it or is it okay to keep it limited to the sink's implementation as done in the PR?

- Add docker_compose_iceberg_rest.yml for tabulario/iceberg-rest catalog backed by the existing MinIO fixture - Extend ClickHouseCluster with with_iceberg_rest=True support (setup_iceberg_rest_cmd, iceberg_rest_host/port/ip attrs) following the same pattern as with_minio/with_azurite - Add tests/integration/test_iceberg_sink/ with three test cases: - basic write: INSERT succeeds and data is readable via table() - retry settings accepted: commit succeeds with non-default retry knobs - multiple batches: sequential INSERTs produce correct row count

…eam sink" This reverts commit 24b0b99.

- Add docker_compose_iceberg_rest.yml for tabulario/iceberg-rest catalog backed by the existing MinIO fixture - Extend ClickHouseCluster with with_iceberg_rest=True support (setup_iceberg_rest_cmd, iceberg_rest_host/port/ip attrs) following the same pattern as with_minio/with_azurite - Add tests/integration/test_iceberg_sink/ with four test cases: - basic write: INSERT succeeds, data readable via table() - retry settings accepted: commit succeeds with non-default retry knobs - multiple batches: sequential INSERTs produce correct row count - conflict retry: proxy injects one 409, Proton retries and succeeds (verifies the retry path end-to-end without Glue compaction dependency)

…eam sink" This reverts commit b821b86.

yuzifeng1984 · 2026-07-08T07:18:01Z

@burnerlee The dedicated retry logic for iceberg commit looks fine. Can you please do the tests and make sure recreated snapshot work correctly when 409 occurs?

burnerlee · 2026-07-10T03:18:48Z

@burnerlee The dedicated retry logic for iceberg commit looks fine. Can you please do the tests and make sure recreated snapshot work correctly when 409 occurs?

sure, I am having some trouble setting up the testing suite on mac - but let me figure this out by the weekend

- Remove private fetchManifestList overload; rename static param to logger_ for consistency - Lower commit retry defaults to num_retries=4 and min_wait_ms=100 to match Java Iceberg commit.retry.* defaults

Add integration test suite for the Iceberg external stream sink, and fix three bugs uncovered while running those tests locally against MinIO: 1. **DatabaseIceberg::createTable path-style fix** — The storage_endpoint HTTP URL was parsed assuming AWS virtual-hosted format (Bucket.s3.Region.amazonaws.com). MinIO uses path-style URLs like http://host:port/bucket/path, so the bucket was incorrectly extracted from the hostname. Now detects path-style vs virtual-hosted and handles both. 2. **Iceberg region propagation** — The `region` setting from ExternalStreamSettings was never forwarded to S3 auth_settings, causing the AWS SDK to omit a force_region and fall back to AWS endpoint resolution even with a custom endpoint. Now propagated. 3. **getObjectInfo no-body 404** — AWS CRT S3 client returns a generic "No response body" error for MinIO's body-less HTTP 404 on HeadObject. `objectExists` now treats this message as a not-found rather than a hard error. Test infra changes: - Add `with_iceberg_rest` support to cluster.py (Iceberg REST catalog compose, IP tracking, startup wait) - Fix docker-compose v2 container name separator (underscore → hyphen) - Set config_root_name="proton" and fix XML tag rewriting to target only <clickhouse> tags, not every occurrence of the word - Respect DOCKER_HOST env for docker SDK socket (needed for Colima/OrbStack) - Add Proton-specific config overrides: data paths, checkpoint path, tcp_port=9000, url_scheme_mappers (s3://→MinIO HTTP) - Recognise `proton` binary basename as clickhouse-compatible client - New docker_compose_iceberg_rest.yml; update docker_compose_minio.yml for newer MinIO API (MINIO_ROOT_USER/PASSWORD)

- Revert deleted comment in docker_compose_minio.yml - Restore @stacktraces_on_timeout_decorator accidentally removed from client.py - Revert config_root_name default back to "clickhouse" to avoid breaking all existing tests; pass config_root_name="proton" explicitly in the Iceberg test fixture only - Move url_scheme_mappers (s3://→MinIO HTTP) from shared 0_common_instance_config.xml to a new test-scoped configs/config.d/s3_url_mapper.xml so it only applies to the Iceberg sink test

burnerlee · 2026-07-15T13:39:20Z

hey @yuzifeng1984 sorry for the delay. Had some hard time setting up the integration tests on local - but now they're working. could you review once

burnerlee added 4 commits July 4, 2026 19:46

add retry mechansim in iceberg sink

ab3aae6

fix(iceberg): move base/sleep.h include after Common headers

6b2176b

revert

c60a501

burnerlee added 4 commits July 4, 2026 20:09

Revert "test(iceberg): add integration tests for Iceberg external str…

4d6ce74

…eam sink" This reverts commit 24b0b99.

Revert "test(iceberg): add integration tests for Iceberg external str…

6a54dd3

…eam sink" This reverts commit b821b86.

yuzifeng1984 reviewed Jul 8, 2026

View reviewed changes

Comment thread src/Storages/ExternalStream/Iceberg/Iceberg.cpp Outdated

yuzifeng1984 reviewed Jul 8, 2026

View reviewed changes

Comment thread src/Core/Settings.h Outdated

refactor(iceberg): address review feedback on commit retry

51c6fc5

- Remove private fetchManifestList overload; rename static param to logger_ for consistency - Lower commit retry defaults to num_retries=4 and min_wait_ms=100 to match Java Iceberg commit.retry.* defaults

burnerlee force-pushed the feat/iceberg-commit-retries branch from 3719345 to 51c6fc5 Compare July 10, 2026 03:26

burnerlee added 2 commits July 15, 2026 00:30

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

feat(iceberg): add commit retry with exponential backoff on 409 conflict#1206

feat(iceberg): add commit retry with exponential backoff on 409 conflict#1206
burnerlee wants to merge 11 commits into
timeplus-io:developfrom
burnerlee:feat/iceberg-commit-retries

burnerlee commented Jul 4, 2026 •

edited

Loading

Uh oh!

burnerlee commented Jul 4, 2026

Uh oh!

Uh oh!

yuzifeng1984 commented Jul 8, 2026 •

edited

Loading

Uh oh!

Uh oh!

burnerlee commented Jul 10, 2026

Uh oh!

burnerlee commented Jul 15, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Uh oh!

Conversation

burnerlee commented Jul 4, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Core changes (commit retry)

Bug fixes (found while writing tests)

Integration tests

Uh oh!

burnerlee commented Jul 4, 2026

Uh oh!

Uh oh!

yuzifeng1984 commented Jul 8, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

burnerlee commented Jul 10, 2026

Uh oh!

burnerlee commented Jul 15, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

burnerlee commented Jul 4, 2026 •

edited

Loading

yuzifeng1984 commented Jul 8, 2026 •

edited

Loading