Partition export per part local backoff policy by arthurpassos · Pull Request #1984 · Altinity/ClickHouse

arthurpassos · 2026-06-29T14:10:58Z

Changelog category (leave one):

Improvement

Changelog entry (a user-readable short description of the changes that goes to CHANGELOG.md):

Implements a local backoff policy for export partition - helps with transient and local failures.,

It is controlled by export_merge_tree_partition_retry_initial_backoff_seconds and export_merge_tree_partition_retry_max_backoff_seconds.

Removes export_merge_tree_partition_max_retries as it would conflict with a local backoff policy implementation. We already have a task time out anyways. The only limitation of relying on the task time out is that non retryable errors would be retried until it times out. Not ideal, so I implemented an err classification method that will classify between retryable and non retryable. Non retryable will fail the task immediately.

Documentation entry for user-facing changes

...

CI/CD Options

Exclude tests:

Regression jobs to run:

chatgpt-codex-connector

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 4d576fefe4

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

Open a pull request for review
Mark a draft as ready
Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

chatgpt-codex-connector · 2026-06-29T14:19:07Z

+        manifest.retry_initial_backoff_ms = json->getValue<size_t>("retry_initial_backoff_ms");
+        manifest.retry_max_backoff_ms = json->getValue<size_t>("retry_max_backoff_ms");


Default backoff fields for old export manifests

When a node upgrades with an export created by the previous code still under /exports, that metadata.json does not contain these two new fields because older manifests only wrote max_retries. ExportReplicatedMergeTreePartitionManifest::fromJsonString is used by the manifest updater before it can clean up or kill tasks, and getValue throws on missing keys, so one old in-flight export makes poll fail repeatedly and prevents this table's export state from progressing until the ZooKeeper entry is removed manually. Please read these fields with defaults when absent, similar to the existing allow_lossy_cast compatibility handling.

Useful? React with 👍 / 👎.

github-actions · 2026-06-30T13:54:29Z

Workflow [PR], commit [8e549de]

arthurpassos · 2026-06-30T18:53:18Z

@codex review, do not worry about backwards compatibility

chatgpt-codex-connector · 2026-06-30T19:05:04Z

Codex Review: Didn't find any major issues. Bravo.

Reviewed commit: 141d1141fb

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

Open a pull request for review
Mark a draft as ready
Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

ianton-ru · 2026-07-02T17:36:23Z

    DECLARE(Bool, export_merge_tree_partition_force_export, false, R"(
 Ignore existing partition export and overwrite the zookeeper entry
 )", 0) \
-    DECLARE(UInt64, export_merge_tree_partition_max_retries, 3, R"(


In common case removed settings are not removed completely, added in OBSOLETE_SETTINGS list for backward compatibility. Why not now?

Huh.. mostly because nobody is using this feature, but I suppose you are right. let me fix that

Should be fixed

ianton-ru · 2026-07-02T17:52:41Z

+        if (initial > headroom)
+            return max_backoff_seconds;
+
+        return std::min(initial << shift, max_backoff_seconds);


Up to 2^64-1 seconds (~585 billion years)? :)

Why rush? :)

Up to max_backoff_seconds, which defaults to 300s (export_merge_tree_partition_retry_max_backoff_seconds)

registerLocalBackoff might be a problem, tho - I'll check after the antalya meeting

registerLocalBackoff seems to be fine as well

ianton-ru · 2026-07-02T18:01:50Z

+        backoff.attempts, manifest.retry_initial_backoff_seconds, manifest.retry_max_backoff_seconds);
+    const auto now = time(nullptr);
+    /// Clamp so a huge configured back-off cannot overflow time_t (now is a normal wall-clock value).
+    const size_t headroom = static_cast<size_t>(std::numeric_limits<time_t>::max() - now);


time_t is signed, size_t is unsigned. If we allow all possible ranges, may get something unexpected. Mya be set hardcoded limit in billion years?

std::numeric_limits<time_t>::max() - now can never be negative tho

mkmkme

I really like the fact that you also changed some exception codes to be more specific :)

mkmkme · 2026-07-03T08:52:56Z

+    using LocalBackoffMap = std::unordered_map<TransactionID, PartNameToBackOffMap>;
+
+    mutable std::mutex local_backoff_mutex;
+    LocalBackoffMap local_backoff;


I'd add TSA_GUARDED_BY(local_backoff_mutex) here

Suggested change

LocalBackoffMap local_backoff;

LocalBackoffMap local_backoff TSA_GUARDED_BY(local_backoff_mutex);

Selfeer · 2026-07-06T09:19:56Z

@arthurpassos can you check if this is reasonable? #2005

I was thinking about the statuses that should/should not be retryable and came up with this.

Selfeer · 2026-07-06T12:57:51Z

Before the testing is finished, here is the latest audit review results for now:

AI audit note: This review comment was generated by AI (gpt-5.3-codex).

Audit update for PR #1984 (Partition export per part local backoff policy):

Confirmed defects:

Medium: Backward-incompatible manifest parsing can stall export-task reconciliation after upgrade.
- Impact: A replica running this PR can throw while reading pre-upgrade export metadata, which aborts poll early and can leave export discovery/cleanup/status convergence stale; killExportPartition can also fail when it scans ZooKeeper entries.
- Anchor: src/Storages/ExportReplicatedMergeTreePartitionManifest.h / ExportReplicatedMergeTreePartitionManifest::fromJsonString; src/Storages/MergeTree/ExportPartitionManifestUpdatingTask.cpp / ExportPartitionManifestUpdatingTask::poll; src/Storages/StorageReplicatedMergeTree.cpp / killExportPartition.
- Trigger: Rolling upgrade with at least one in-flight export entry created by the old format (max_retries present, retry_initial_backoff_seconds and retry_max_backoff_seconds absent).
- Why defect: fromJsonString unconditionally reads retry_initial_backoff_seconds and retry_max_backoff_seconds via json->getValue, and callers in poll/killExportPartition do not handle this as a per-entry compatibility case.
- Fix direction (short): Add backward-compatible fallback in fromJsonString (or guarded read at call sites) so old metadata remains parseable and does not abort whole-task scanning.
- Regression test direction (short): Add an upgrade/compatibility integration test that seeds a legacy metadata.json entry and verifies poll and killExportPartition keep working on the new binary.

Coverage summary:

Scope reviewed: Retry/back-off scheduler flow, commit/part failure classification, ZooKeeper state transitions, manifest/schema evolution, system table exposure, and new integration/stateless tests.
Categories failed: Serialization/upgrade compatibility (metadata.json field evolution).
Categories passed: Local back-off thread-safety, lock release and status transition ordering, fail-open/fail-closed classification intent, rollback on Keeper multi failure, exception accounting race hardening, and non-retryable fast-fail paths.
Assumptions/limits: Static audit only (no runtime execution in this review); severity reflects observed code paths and realistic rolling-upgrade behavior.

arthurpassos · 2026-07-07T13:05:19Z

Medium: Backward-incompatible manifest parsing can stall export-task reconciliation after upgrade.

hmm.. so far I was opting for not introducing "backwards compatible code". But since we already released 26.3, it might be a good idea to make this safer

Selfeer · 2026-07-07T20:12:34Z

No issue from my side do block this PR. The failes described are not considered bugs or issues here. @CarlosFelipeOR could you please fix these tests afterwards for the sake of the clean CI in the future.

PR #1984 CI Failure Triage

PR: Altinity/ClickHouse#1984 — Partition export per part local backoff policy
Base: antalya-26.3 ← export-partition-retry-backoff
Commit: 8e549de4f747a121b90a36b0c0143f00b8854e22
Workflow run: 28871008590
Report: ci_run_report.html

Verdict

All 11 CI failures are directly caused by this PR's changes. They are not flaky,
not infrastructure, and not cascade failures. Every failure is a regression test whose
expectations conflict with the PR's intended new behavior (error re-classification and
the removal of the fixed retry budget). The tests need to be updated to match the new
behavior (or the two behavior decisions below need to be confirmed as intended).

Everything else in the run is green: s3_export_part, s3_export_partition, all
Integration tests (including the export-partition integration tests modified by the PR),
all Stateless tests (including the modified 03572_* export tests), Fast test, fuzzers,
stress, and builds.

Summary

Category	Count	Tests
PR-caused regression	11	iceberg export partition casting (8) + transactions (3)
Cascade failures	0	—
Pre-existing flaky	0	—
Infrastructure (known/BROKEN, unrelated)	7	see "Known/unrelated" below

The CI report header agrees: New Fails in PR (0), Checks New Fails (0),
Regression New Fails (11), Checks Known Fails (7).

All 11 new failures are in the single job Regression release iceberg_2, all under
/iceberg/export partition/.

Root cause analysis

The PR makes two intended behavioral changes, and both are exactly what the failing
tests assert against.

Change A — lossy-cast rejection error code `BAD_ARGUMENTS` (36) → `INCOMPATIBLE_COLUMNS` (122)

PR diff (src/Storages/.../ExportPartitionUtils.cpp write/validation path):

-                throw Exception(ErrorCodes::BAD_ARGUMENTS,
+                throw Exception(ErrorCodes::INCOMPATIBLE_COLUMNS,
                     "Cannot export to {}: column '{}' requires a lossy cast from {} to {}, "

The regression tests assert a synchronous rejection with client exit code 36
(iceberg/tests/export_partition/steps/casting.py, BAD_ARGUMENTS = 36). The server now
returns code 122 (INCOMPATIBLE_COLUMNS). The rejection message still contains
"lossy cast", so only the exit-code assertion (exitcode should be 36) fails.

Observed in the log:

Code: 122. DB::Exception: ... column 'id' requires a lossy cast from UInt64 to Int32,
which may change values. Set `export_merge_tree_part_allow_lossy_cast = 1` ...
(INCOMPATIBLE_COLUMNS)
...
✘ Fail exitcode should be 36

Affected tests (8):

/iceberg/export partition/ice catalog/casting/out_of_bounds/UInt64 above INT64_MAX rejected without allow_lossy_cast
/iceberg/export partition/no catalog/casting/out_of_bounds/UInt64 above INT64_MAX rejected without allow_lossy_cast
/iceberg/export partition/ice catalog/casting/out_of_bounds/Int64 above INT32_MAX rejected without allow_lossy_cast
/iceberg/export partition/no catalog/casting/out_of_bounds/Int64 above INT32_MAX rejected without allow_lossy_cast
/iceberg/export partition/ice catalog/casting/out_of_bounds/Int64 below INT32_MIN rejected without allow_lossy_cast
/iceberg/export partition/no catalog/casting/out_of_bounds/Int64 below INT32_MIN rejected without allow_lossy_cast
/iceberg/export partition/ice catalog/casting/lossy/UInt64 narrows to Int32 without allow_lossy_cast
/iceberg/export partition/no catalog/casting/lossy/UInt64 narrows to Int32 without allow_lossy_cast

Change B — new retryable / non-retryable error classification

The PR removes export_merge_tree_partition_max_retries (now obsolete) and adds
ExportPartitionUtils::isNonRetryableExportError(code). The non-retryable set includes
BAD_ARGUMENTS (36) and INCOMPATIBLE_COLUMNS (122), among others. Non-retryable
commit failures transition the export task straight to FAILED instead of retrying.

The transactions/commit survives pre-publish failure (non-retry cleanup) scenario arms
the iceberg_writes_non_retry_cleanup failpoint and expects the export to retry and
reach COMPLETED. The failpoint throws code 36 (BAD_ARGUMENTS), which the PR now
treats as non-retryable, so the task ends FAILED.

Test assertion:

AssertionError: Expected status 'COMPLETED' ... but got 'FAILED'

Server log (all 3 catalog variants show the same):

ExportPartition: Commit failure recorded for .../exports/2020_... (code 36),
task transitioned to FAILED (non-retryable)
ExportPartition scheduler task: Commit for ... transitioned to FAILED due to
non-retryable error (code 36)

Affected tests (3):

/iceberg/export partition/glue catalog/transactions/commit survives pre-publish failure （non-retry cleanup）
/iceberg/export partition/ice catalog/transactions/commit survives pre-publish failure （non-retry cleanup）
/iceberg/export partition/no catalog/transactions/commit survives pre-publish failure （non-retry cleanup）

Points to confirm with the PR author

Lossy-cast error code change (36 → 122). Confirm INCOMPATIBLE_COLUMNS is the
intended user-facing code for lossy-cast rejection, then update the casting regression
tests to assert exit code 122 (message assertion already passes).
BAD_ARGUMENTS (36) is now non-retryable. The commit survives pre-publish failure (non-retry cleanup) scenario was written on the assumption that this
pre-publish failure is transient and gets retried to success. With code 36 now
non-retryable, the task fails immediately. Decide whether:
- the test should be updated to expect FAILED (i.e. the failpoint models a
  non-retryable error), or
- the failpoint/classification should use a retryable code so the "survives / retries
  to completion" contract still holds.

Both are behavior/expectation questions introduced by this PR — neither is a flaky or
infra failure.

Known / unrelated failures (not caused by the PR)

These are pre-existing BROKEN/known issues listed under Checks Known Fails (7) and are
infrastructure or already-tracked problems, unrelated to this PR:

test_dirty_pages_force_purge — KNOWN, Altinity#1369
test_backup_restore_on_cluster/test_different_versions — INVESTIGATE (NETLINK_ERROR)
03441_deltalake_clickhouse_public_datasets — INVESTIGATE (S3 sometimes unreachable) ×2
03441_deltalake_clickhouse_virtual_columns — INVESTIGATE (S3 sometimes unreachable) ×2
00157_cache_dictionary — KNOWN (timeout on slow Altinity runners, passes upstream)

Stateless tests (amd_debug, parallel) shows Failed: 1, but the run reports
Checks New Fails (0) — it is not a new failure introduced by this PR.

Recommendation

Do not treat these as flaky and re-run — they will fail deterministically because
they reflect the PR's intended behavior change.
Update the affected iceberg export-partition regression tests (casting exit code, and
the non-retry-cleanup transaction scenario) to match the new error code and
retry semantics, after confirming the two decisions above with the PR author.

arthurpassos · 2026-07-08T11:52:38Z

@ianton-ru @mkmkme can I get a 2nd pass?

mkmkme

New changes LGTM!

first vibe coded version of local backoff policy

4d576fe

chatgpt-codex-connector Bot reviewed Jun 29, 2026

View reviewed changes

svb-alt assigned arthurpassos Jun 29, 2026

update tests

fe5f90f

arthurpassos changed the base branch from rw_lock_for_partition_export to antalya-26.3 June 30, 2026 13:53

Merge branch 'antalya-26.3' into export-partition-retry-backoff

4d2d5e6

arthurpassos changed the title ~~first vibe coded version of local backoff policy~~ Partition export per part local backoff policy Jun 30, 2026

arthurpassos added 4 commits June 30, 2026 11:55

change a few err codes

751ba3c

add test that covers locality of backoff

70c70db

fix fast test and tests

17309a1

make it more readable

141d114

arthurpassos added 9 commits July 1, 2026 10:58

fix some tests

d081723

add more exceptions to non retryable errors..

5230134

turn setting into seconds instead of ms

8e92d62

change backoff max default to 5min

ec8b42c

fix race condition

da542ad

Merge branch 'antalya-26.3' into export-partition-retry-backoff

c1a539f

remove unnecessary test and reduce test timeout

c997bb6

update tests

cadbc32

rmv useless test

e192c4d

arthurpassos added antalya port-antalya PRs to be ported to all new Antalya releases antalya-26.3 labels Jul 2, 2026

ianton-ru reviewed Jul 2, 2026

View reviewed changes

mkmkme reviewed Jul 3, 2026

View reviewed changes

arthurpassos added 2 commits July 3, 2026 09:21

add tsa mutex

908bce0

add a few more err codes

4c94cfd

make setting obsolete

f89bdce

Selfeer mentioned this pull request Jul 6, 2026

EXPORT PARTITION retries permanent S3 credential errors until the task timeout #2005

Open

arthurpassos requested review from ianton-ru and mkmkme July 7, 2026 13:04

arthurpassos added 2 commits July 7, 2026 10:29

backwards compatible backoff policy

8ea161b

Merge branch 'antalya-26.3' into export-partition-retry-backoff

8e549de

Selfeer added the verified Approved for release label Jul 7, 2026

mkmkme approved these changes Jul 8, 2026

View reviewed changes

		manifest.retry_initial_backoff_ms = json->getValue<size_t>("retry_initial_backoff_ms");
		manifest.retry_max_backoff_ms = json->getValue<size_t>("retry_max_backoff_ms");

	LocalBackoffMap local_backoff;
	LocalBackoffMap local_backoff TSA_GUARDED_BY(local_backoff_mutex);

Uh oh!

Conversation

arthurpassos commented Jun 29, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Changelog category (leave one):

Changelog entry (a user-readable short description of the changes that goes to CHANGELOG.md):

Documentation entry for user-facing changes

CI/CD Options

Exclude tests:

Regression jobs to run:

Uh oh!

chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

💡 Codex Review

Uh oh!

chatgpt-codex-connector Bot Jun 29, 2026

Choose a reason for hiding this comment

Uh oh!

github-actions Bot commented Jun 30, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

arthurpassos commented Jun 30, 2026

Uh oh!

chatgpt-codex-connector Bot commented Jun 30, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mkmkme left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Uh oh!

Selfeer commented Jul 6, 2026

Uh oh!

arthurpassos commented Jul 7, 2026

Uh oh!

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

Uh oh!

PR #1984 CI Failure Triage

Verdict

Summary

Root cause analysis

Change A — lossy-cast rejection error code BAD_ARGUMENTS (36) → INCOMPATIBLE_COLUMNS (122)

Change B — new retryable / non-retryable error classification

Points to confirm with the PR author

Known / unrelated failures (not caused by the PR)

Recommendation

Uh oh!

arthurpassos commented Jul 8, 2026

Uh oh!

mkmkme left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

arthurpassos commented Jun 29, 2026 •

edited

Loading

github-actions Bot commented Jun 30, 2026 •

edited

Loading

Selfeer commented Jul 6, 2026 •

edited

Loading

Selfeer commented Jul 7, 2026 •

edited

Loading

Change A — lossy-cast rejection error code `BAD_ARGUMENTS` (36) → `INCOMPATIBLE_COLUMNS` (122)