Add live point-in-time logical backup producer#1128
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Current signed HEAD is Verification:
Please review the latest head. @codex review |
|
✅ Action performedReview finished.
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request implements Phase 1 of the logical backup feature, enabling point-in-time consistent logical backups from a running elastickv cluster. It introduces the elastickv-backup CLI tool, adds server-side configurations for active backup pins, implements the live backup producer lifecycle (including streaming and renewal), and updates design documents and runbooks. The review feedback highlights a potential nil pointer dereference on Windows when loading system certificates, portability issues with using os.Link for manifest publication on filesystems that do not support hard links, and noisy warning logs during graceful shutdown due to unsuppressed context cancellation errors.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
|
Updated the latest head
Verification:
Caller audit for the manifest error-semantics change found one production caller ( @codex review |
830d645 to
6a85349
Compare
d5a5934 to
83f3a9f
Compare
|
History-only author-compliance repair completed at
No runtime or source-tree semantics changed. CI is running on the reconstructed head. @codex review |
|
Current reconstructed head |
|
Latest signed head |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 83f3a9f6aa
ℹ️ 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".
|
Current signed HEAD Evidence:
Verification:
Caller audit for the @codex review |
There was a problem hiding this comment.
💡 Codex Review
elastickv/internal/backup/live.go
Lines 66 to 67 in 56277d2
Exclude DynamoDB generation counters from scopes
When a DynamoDB table has been deleted, the live adapter leaves the !ddb|meta|gen| counter behind while the table schema is gone, and the decoder's HandleTableGen is a no-op. Classifying that counter as a scoped table makes BeginBackup/ListAdaptersAndScopes select a deleted table and lets the live manifest publish dynamodb.tables for a scope that has no _schema.json; the native restore path then emits no table even though the manifest says it was dumped. Treat this like the other generation counters that are already unscoped, or only count it when the schema record is present.
ℹ️ 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".
|
Latest signed HEAD
Verification:
Caller audit for @codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7d4834f121
ℹ️ 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".
|
Evidence for current HEAD
Validation:
Requesting a fresh latest-head review. @codex review |
ed0498d to
53e6402
Compare
|
Rebase evidence for current HEAD
Post-rebase validation:
The broad unfiltered adapter suite reached its 600-second timeout in Raft integration cleanup; the focused backup/forwarding adapter suite passed and GitHub CI is now running on this head. Requesting a fresh latest-head review after the dependency rebase. @codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 53e64025e8
ℹ️ 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".
| if object.manifest == nil || (bucket.activeGen != 0 && object.generation != bucket.activeGen) { | ||
| continue |
There was a problem hiding this comment.
When a bucket has blob rows that are intentionally excluded from the native dump — for example chunks from an in-flight multipart upload, stale upload attempt, or old bucket generation — this retained-count path drops them, but the BeginBackup/ListAdaptersAndScopes baseline still counts every !s3|blob|... row for the bucket via ScopeForKey. That makes validateExpectedLiveCounts see a shortfall and abort otherwise valid live backups once enough excluded chunks exist, even though the default producer mode is documented to omit incomplete uploads/orphans. Please make the expected-count side use the same retained S3 denominator or otherwise exclude those blob rows before comparison.
Useful? React with 👍 / 👎.
| if err := d.writeEffectiveDDBItems(itemsDir, st.name, hashKey, rangeKey, items); err != nil { | ||
| return err | ||
| } | ||
| st.retained = 1 + retainedDDBItemRecords(st.itemsByGen, emitOrder) |
There was a problem hiding this comment.
When an active table still has leftover item rows from a non-active/non-migration generation, the baseline scan counts those !ddb|item|... records for the table via ScopeForKey, but this retained count only includes generations selected for output. If the stale generation has more than the shortfall tolerance, validateExpectedLiveCounts aborts an otherwise valid backup even though the encoder intentionally drops stale-generation rows to avoid resurrecting deleted data. Please make the baseline use the same generation-filtered denominator or keep these counts comparable.
Useful? React with 👍 / 👎.
| if st.genSeen { | ||
| count++ | ||
| } | ||
| visible, _ := filterStaleGenMessages(st.messages, st.activeGen) |
There was a problem hiding this comment.
When a queue has residual old-generation messages after a PurgeQueue/DeleteQueue while the reaper is still cleaning up, the BeginBackup/ListAdaptersAndScopes baseline counts every !sqs|msg|data|... row for that queue, but this retained count filters those stale messages out. Once the stale rows exceed the tolerated shortfall, live backup fails even though flushQueue deliberately drops those messages to preserve the current user-visible queue. Please apply the same generation filtering to the expected-count baseline or otherwise compare like with like.
Useful? React with 👍 / 👎.
| if st.sawWide { | ||
| count += uint64(len(st.members)) | ||
| } else { | ||
| count += boolRecordCount(st.legacySeen) |
There was a problem hiding this comment.
When Redis has many zsets in the dual legacy+wide-column state left by migration, ScopeForKey still counts each !redis|zset|... legacy blob in the BeginBackup/ListAdaptersAndScopes baseline, but this retained-count path drops that record as soon as any wide member exists because the live read path prefers the wide rows. That leaves actual counts lower than the baseline and can make validateExpectedLiveCounts abort otherwise valid live backups of migrated Redis data; please exclude stale legacy zset blobs from the baseline or count them consistently.
Useful? React with 👍 / 👎.
1 participant
Summary
Stack
Risk
The data path is opt-in through the new backup CLI and admin RPCs. The main correctness risks are publishing a completion marker after pin loss and reading from a changing route catalog; both are covered by pinned-route and renewal-quiescence tests.
Verification
go test ./internal/backup ./cmd/elastickv-backup -count=1go test -race ./internal/backup -run 'Test(RunLiveBackup|ProducerCrash|CrossAdapterConsistency|ExpectedKeysBaseline)' -count=1go test ./adapter -run 'Test(BeginBackup|RenewBackup|StreamBackup|ListBackup|BackupProtocol|InternalForwardAdminProposal)' -count=1go test ./kv -run 'Test(Backup|BeginBackup|Pin|BackupScanner|ShardStoreScanAt_RoutesFilesystem)' -count=1make -C proto genfollowed by a clean generated diffgolangci-lint run ./... --timeout=15m --allow-parallel-runners -j 2git diff --checkdocs/design/2026_04_29_proposed_logical_backup.md