Fix incremental restore to survive SchemeShard reboots and handle shard failures#36
Open
maximyurchuk wants to merge 28 commits into
Open
Fix incremental restore to survive SchemeShard reboots and handle shard failures#36maximyurchuk wants to merge 28 commits into
maximyurchuk wants to merge 28 commits into
Conversation
Code review of the prior commits in this PR surfaced two correctness defects in the shard-failure retry path plus several cleanup items. Correctness fixes: 1. RetryNeeded overwrite race in CheckForCompletedOperations. Two TTxProgressIncrementalRestore runs can interleave: the first erases the failure flag from FailedIncrementalRestoreOperations and sets RetryNeeded=true; if a second run arrives before the retry branch executes, it sees no failures and overwrites RetryNeeded back to false, silently losing the retry signal. Changed assignment to |=. 2. Retry budget. The retry loop had no terminal condition — a permanently broken shard caused infinite scan re-creation. Added per-incremental retry counter (transient, reset on advance) capped at 5 attempts. On exhaustion, state is set to EState::Failed and persisted; get/list APIs report PROGRESS_DONE with GENERIC_ERROR. Cleanups: - Removed dead 1-arg TEvRunIncrementalRestore constructor (zero callers after the orphan-recovery fix in this PR). - Extracted TrackIncrementalRestoreShardReply helper to deduplicate the ~26-line shard-reply tracking block that was identical between TProposeAtTable::HandleReply and NIncrRestore::TProposedWaitParts::HandleReply. Test coverage: - Added IncrementalRestoreShardFailureTriggersRetry: uses runtime.AddObserver to inject one TEvFinished failure and verifies the retry path recovers data integrity. - Added TestIncrementalRestoreFinalizingStateRecoveryAfterReboot: exercises TTxInit resume from EState::Finalizing via a 5-incremental chain and bucket reboots. - Hardened WaitForRestoreDone with required bool expectRegistered parameter — prior version silently passed when ListRestores returned empty entries, masking real registration regressions.
Add MaxIncrementalRestoreTablesInFlight ImmediateControl (default 32, sentinel -1 = unbounded). The orchestrator now enqueues table and index sub-ops onto PendingTables and DispatchPendingTables drains in waves bounded by the cap, topped up after each completion. Prevents unbounded fan-out on backup collections with many tables/indexes.
Funnel TrackIncrementalRestoreShardReply through TTableOperationState:: RecordShardResult so the first terminal report wins and re-deliveries (DataShard restart replays, schemeshard reboot re-sends, interconnect retransmits) are dropped. Prevents double-counting that would put a shardIdx in both Completed and Failed sets and break AllShardsComplete's sum invariant. Adds a contract unit test.
Line numbers in cross-file comments rot as soon as either file is edited. The adjacent code is self-explanatory.
Add MaxIncrementalRestoreRetriesPerIncremental ImmediateControl (default 50, sentinel -1 = unbounded) to bound the orchestrator's per-incremental retry attempts. Propagate a Retriable bit on TShardOpResult / TEvFinished so non-retriable scan failures (anything other than Done/Term) short- circuit the retry loop instead of burning the budget. Failed restores now report GENERIC_ERROR via the list endpoint instead of the SUCCESS default. Adds integration tests for backoff timing, budget enforcement, unlimited-cap sentinel, non-retriable short-circuit, reboot survival, and concurrent-completion de-duplication. Bumps reboot suite SPLIT_FACTOR to 50 and adds explicit ram/cpu requirements.
Extract the per-entry TBackupCollectionRestore progress/status switch into FillRestoreProgress in schemeshard_restore_incremental_progress.h and call it from both __get and __list, so the two endpoints can't diverge on Failed -> GENERIC_ERROR or progress percent. Trim Retriable bit comments down to one-liners.
Move the ad-hoc "_full" / "_incremental" string literals scattered across backup, restore, and init paths into FullBackupSuffix / IncrementalBackupSuffix constants plus FullBackupDirName / IncrementalBackupDirName builders in schemeshard__backup_collection_common.h. Eliminates the risk of one site drifting from another when the convention ever changes.
…odies Pull out RestoreIncrementalRestoreOpPathStates and ScheduleOrphanedIncrementalRestoreOp from TTxInit::ReadEverything, HandleRetryPath and HandleAllOperationsComplete from TTxProgressIncrementalRestore::Execute, ValidateProposePaths from TNewRestoreFromAtTable::Propose, and TrackSubOpAndExpectedShards reused by both CreateSingleTableRestoreOperation and CreateSingleIndexRestoreOperation. Also tighten the reboot-tests file. Pure refactor — no behavior change. The new TTxInit helper keeps using NBackup::FullBackupDirName / IncrementalBackupDirName so the suffix centralization from 1102753 stays.
Pure comment cleanup across PR-touched files: collapse multi-paragraph block comments to one or two short sentences, drop comments that just paraphrase the next line of code, and move trailing end-of-line comments to the line above. No code or behavior changes.
The InvolvedShards set was populated but never read — cleanup paths key by OperationId. Remove the in-memory set, the TTxInit reload block, and the per-sub-op insert; reserve proto tag 7 in TLongIncrementalRestoreOp (formerly InvolvedShards) to prevent reuse.
Persist EState::Completed/Failed (with FinalStatus + FinalIssues) into Schema::IncrementalRestoreState instead of deleting the row at finalize, so Get/List can return the orchestrator's terminal verdict across reboots. Add FinalizeTxId column so TTxInit can detect a Finalizing op whose finalize sub-op was lost on reboot and reset to Running for re-issue. Route full-only restores (no incrementals) through the same orchestrator so they get a state row and are pollable like incremental restores. Unify Get and List status semantics: outer envelope is always SUCCESS; per-entry inner Status carries the real outcome. FillRestoreProgress now layers the persisted FinalStatus over the SUCCESS default for Completed and over GENERIC_ERROR for Failed. Allow FORGET on Failed restores in addition to Completed/Finalizing. Tests: expand ut_backup_collection (663 lines) and ut_incr_restore_reboots (304 lines) covering durable Completed/Failed across reboot, Finalizing recovery, full-only restore reboot, and FORGET semantics for terminals.
Comment pass: remove dev-process markers (Bug #N, RED on HEAD, T1.x taxonomy) and trim wordy block comments across the restore files. Small follow-ons that surfaced during the pass: - TTxInit: skip RestoreIncrementalRestoreOpPathStates when the persisted state is already Completed or Failed (no point resetting path states for a terminal row). - TTxProgressIncrementalRestore: clear TableOperations along with the rest of the per-incremental tracking on retry. - __get: skip Reply() and SideEffects.Send directly so the per-entry Status set by FillRestoreProgress isn't clobbered by the default SUCCESS Reply path. - Drop unused MaxRetriesPerIncremental constant.
Drop the header-only-helpers framing block and the inline comment in IsScanRetriable; the function body is self-evident. Drop the end-of-line "meaningful only when !Success" on Retriable too.
Disambiguates the helper name now that the backup side will get its own analogous terminal-state persister.
…ExpectedShards Same disambiguation as the previous rename — the backup side will get its own analogous tracker.
Rename DispatchPendingTables, EnqueueIndexesRecursive, and FindTargetTablePath to their PrefixedIncrementalRestore variants so the backup side can grow analogous helpers without name collisions.
Same paraphrase in datashard_active_transaction.h and tx_datashard.proto; the field name plus its sibling Success/Error fields make the meaning obvious.
Move WaitForIncrementalBackupDone and related polling/observer helpers out of ut_incr_restore_reboots and into a new test_incremental_restore.h under ut_helpers, shared with ut_incremental_restore. Lets the backup tracking branch reuse the same primitives instead of forking them.
Reflects the FinalStatus, FinalIssues, and FinalizeTxId columns added in the durable-terminal-state work.
Switch the wire-level scan-termination signal from a single Retriable bool to a typed EOpEndStatus enum (END_UNSPECIFIED, END_SUCCESS, END_TRANSIENT_FAILURE, END_FATAL_FAILURE). DataShard classifies the cause via MapScanStatus; SchemeShard owns the retry policy via the new ShouldRetryIncrementalRestore helper in schemeshard_incremental_- restore_classify.h. Avoids the boolean overload where "is this retriable?" had to encode the difference between transient and fatal in one bit. Touches: TShardOpResult proto, TEvFinished, TSchemaOperation, TDataShard::NotifySchemeshard, the SS-side dispatcher and reboot tests, plus the schema canondata picking up the new state columns.
Drop "Step E:" prefix and the multi-paragraph paraphrase of the reload loop. Trim the function-doc blocks for EnqueueIncrementalRestoreItem and CleanupIncrementalRestoreItems to one short line each. Tighten the Finalizing-recovery rationale comment.
Each multi-line "Plan v4 T-Rn:" / "Test N (integration ...):" header becomes a single sentence describing what the test asserts. Also drop the TxProgressNotExecutedForFullBackupOnly tombstone block — git history covers that.
Drop the EOL note next to reserved 7 in TLongIncrementalRestoreOp and the 3-line Column<7> placeholder block in schemeshard_schema.h. The proto still reserves tag 7; schemeshard_schema.h doesn't need a matching //-comment.
FindPtr returns nullptr when the entry is absent, which is exactly what the second branch passed explicitly. Collapse the if/else into the single call.
Collapse the multi-line blocks at the retry path, finalize transition, per-item cleanup, dispatch cap, CleanupIncrementalRestoreItems body, and the AllocateResult tx into one or two short sentences each. Drop the inline doc that paraphrased TTxProgressIncrementalRestoreAllocate- Result's behavior.
The "tier" naming came from the design plan; in code, the distinction is just "per-incremental retry budget" vs "allocator retry". Use those terms directly so future readers don't need the spec context.
Drop redundant doc on BackupCollectionPathId / OriginalOperationId (the names speak for themselves), collapse the multi-paragraph TItem / PendingRequest preamble, and strip the trailing EOL note on WaitTxId.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes ydb-platform#22274
Related: ydb-platform#21487, ydb-platform#31068
Changelog entry
...
Changelog category
Description for reviewers
...