Repository navigation
Retry old-epoch backup mutation-log uploads - #13896
tclinkenbeard-oai merged 6 commits into
Conversation
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
saintstack
left a comment
There was a problem hiding this comment.
From the local LLM:
The retry predicate likely misses the production destination. It retries only io_error/io_timeout, which originate in the IAsyncFile layer — file:// destinations and simulated disk faults. The blob-store upload path never throws them (BlobStoreCommon.cpp:985-1010 throws http_request_failed, connection_failed, timed_out; the one io_error in S3BlobStore.cpp:1140 is read-path). The PreserveFailurePolicy test deliberately asserts http_request_failed is not retried, so this was a conscious choice with a decent rationale — the blob store retries internally. But for S3-backed backups the stall the PR describes appears to remain, and the unit tests can't reveal that because they inject the codes directly into the mock.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
tclinkenbeard-oai
left a comment
There was a problem hiding this comment.
Generated by Codex.
What problem is this PR trying to solve? (ELI10)
A backup worker copies the database’s change history into backup storage. An old-epoch worker handles history from an earlier database generation. If its upload fails, it can exit while unfinished backups keep waiting. Its failure deliberately does not restart database recovery.
For example, illustratively, a temporary storage outage can exhaust upload retries and stop that worker. Even after storage recovers, backup progress can remain stuck and the old logs stay retained.
How does this PR solve the problem? (ELI10 and review walkthrough)
Keep the old worker alive and retry the same chunk of history using fresh files. If storage accepted a file but its success reply was lost, the retry must preserve the same mutation contents so either copy is safe to restore.
- Snapshot the batch: retain the selected message prefix and freeze recipient ranges, versions, and block size. New recipients get another pass before that prefix is discarded.
- Retry destination failures: retry only an unstopped old-epoch worker. Current-epoch failures and cancellation still propagate. Each attempt opens fresh files.
- Preserve clear-range ordering: writes to each file are chained, retaining their bytes across waits. Frozen boundaries keep retries consistent when another backup stops. Existing restore code filters duplicate log intervals.
- Advance progress after saving: only after saving succeeds does the caller discard messages, record progress, and permit old log data to be released.
What is it trying to do?
Recover from transient destination failures during old-epoch partitioned-log uploads without restarting transaction recovery, losing mutations, or changing current-epoch failure handling.
Is it correct?
Yes, based on source inspection at head dbb02ca4eee1fb7af25c0fc7ed1c970320b98f47, against base a003925f03ef78d5270da134e47d2422985cc440.
I inspected error propagation, async ownership and cancellation, recipient changes, duplicate restore handling, progress ordering, hot-path costs, and regression tests. No persisted format or network schema changes were introduced. The tests exercise fresh-file retries, partial writes, ambiguous completion, changing recipients, and stopping/cancellation.
No builds, tests, or simulations were run for this review. Current-head CI has three passing checks and seven pending jobs, with no reported failures.
Are there bugs?
I did not find any correctness bugs.
The earlier review’s missing-blob-errors concern was valid for the older revision and is addressed here: the retry predicate now includes http_request_failed, connection_failed, timed_out, and lookup_failed, matching the blob request failure outputs.
Are there omissions?
None that I think block this. The main remaining coverage gap is the complete worker/progress path during a real blob-storage outage: the added tests exercise helpers, while the existing partitioned-backup workload uses a file destination. Replacing a dead process and interrupting a permanently hung destination remain explicitly outside this change’s scope.
Are there better ways of doing things?
No alternative warrants changing this approach. Per-file append chaining adds bookkeeping but preserves ordering and concurrent writes across different files.
Should this CL be LGTMd?
Yes, LGTM based on code inspection. Retry policy, ownership, duplicate handling, and durable-progress ordering look sound. The highest remaining risk is distributed integration behavior during destination outages; current-head CI is still pending.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
@saintstack @neethuhaneesha can you have a look? |
saintstack
left a comment
There was a problem hiding this comment.
(Sorry so late reviewing this)
LGTM. #14240 is related. I think they compliment. It adds the 'replacing a terminated process' you note in here (But not 'destination hangs permanently')
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Result of foundationdb-pr-clang-ide on Linux RHEL 9
|
Result of foundationdb-pr-macos-m1 on macOS 14.x
|
This comment has been minimized.
This comment has been minimized.
Result of foundationdb-pr-clang on Linux RHEL 9
|
Result of foundationdb-pr-clang-arm on Linux RHEL 9
|
Result of foundationdb-pr on Linux RHEL 9
|
Result of foundationdb-pr-cluster-tests on Linux RHEL 9
|
Result of foundationdb-pr-macos on macOS 14.x
|
Summary
An I/O error while writing partitioned backup logs can terminate an old-epoch backup worker. Those failures deliberately do not restart transaction recovery, so the old log generation can remain retained and backup progress can stop indefinitely.
Retry transient file and blob-store request failures for old-epoch mutation-log uploads while retaining the same role and message prefix. Each attempt opens fresh files. Freeze the recipient ranges and encoding boundaries so an ambiguous finish produces byte-identical duplicates, and handle newly observed overlapping backups before discarding messages or advancing durable progress. Preserve current-epoch recovery, cancellation, and displacement behavior.
Add focused regression coverage for file creation, partial append, ambiguous finish, changing recipients, and stopping during an upload or retry.
Validation
fdbserver_backupworker_test: 8/8 in native mode and 8/8 in simulation mode after test consolidation.Earlier validation of the unchanged production logic:
BackupCorrectnessPartitioned.tomlwith buggify and fault injection enabled: passed; backup and restore completed.This change covers transient destination I/O failures. It does not add replacement of an already-terminated process or recovery from a permanently hung destination.