Repository navigation
br, pkg/objstore: add operation metadata to external storage locks (#69231) - #69388
Merged
ti-chi-bot[bot] merged 3 commits intoJun 26, 2026
Merged
ti-chi-bot[bot] merged 3 commits into
ti-chi-bot[bot] merged 3 commits into
Conversation
Signed-off-by: ti-chi-bot <ti-community-prow-bot@tidb.io>
ti-chi-bot
added
do-not-merge/hold
2 of 13 tasks
Member
Author
|
@RidRisR This PR has conflicts, I have hold it. |
|
@ti-chi-bot: ## If you want to know how to resolve it, please read the guide in TiDB Dev Guide. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the ti-community-infra/tichi repository. |
|
Caution Review failedAn error occurred during the review process. Please try again later. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 14
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@br/pkg/operation/BUILD.bazel`:
- Line 9: The dependency declared as `//pkg/objstore` on line 9 of the
BUILD.bazel file is unresolved and prevents the build from succeeding. Since the
PR code resolves lock metadata types from the BR storage package, replace the
`//pkg/objstore` dependency with the correct Bazel target path that points to
the BR storage package containing the lock metadata types needed by this
operation package.
In `@br/pkg/operation/context.go`:
- Line 27: The import statement in the context.go file imports from an
unavailable package path. Replace the broken import
`"github.com/pingcap/tidb/pkg/objstore"` with the correct import path where the
lock metadata types are actually located in this branch. Ensure the corrected
import path resolves without compilation errors.
In `@br/pkg/storage/BUILD.bazel`:
- Around line 130-134: Resolve the merge conflict in the BUILD.bazel file's deps
list by removing the merge conflict markers (<<<<<<, =======, and >>>>>>>) and
keeping the necessary dependencies. The conflict shows two different
dependencies: the gomock dependency from HEAD and the zap dependency from the
merged branch. Determine which dependencies are actually needed for this build
target and keep both if they serve different purposes, or keep only the required
one. Ensure the final deps list in the file is clean without any merge markers.
In `@br/pkg/storage/locking_test.go`:
- Around line 80-84: The test file contains unresolved merge conflict markers
that prevent compilation. Resolve all merge conflicts in the locking_test.go
file by replacing the conflicted sections (marked with <<<<<<<, =======,
>>>>>>>) with the correct merged code. For the TryLockRemote function calls, use
the newer API signature from the incoming branch that uses
objstore.TryLockRemote with objstore.LockMetaInput{Hint: ...} instead of the old
storage.TryLockRemote signature. Apply this same pattern to all conflicted
sections at the specified line ranges (80-84, 95-103, 114-135, 172-185, 210-214)
to ensure the entire file reflects the updated lock metadata API.
In `@br/pkg/storage/locking.go`:
- Around line 136-142: The locking.go file contains unresolved merge conflicts
across multiple sections (lines 136-142, 402-454, 531-549, 584-689) with
conflict markers that prevent the file from being parsed and compiled. Locate
and resolve each merge conflict by reviewing the changes between the HEAD
version and the incoming branch (commit 807326b), which adds operation metadata
to external storage locks. For each conflict involving changes like the WalkDir
method signature, WalkOption imports (switching to storeapi.WalkOption), and the
introduction of blockers-related variables, choose the appropriate code path
(typically the incoming branch with the new lock-metadata behavior) and remove
all conflict markers (<<<<<<< HEAD, =======, >>>>>). Ensure the final code is
syntactically correct with consistent method signatures and imports throughout.
In `@br/pkg/stream/BUILD.bazel`:
- Around line 86-90: A cherry-pick conflict exists in the stream_test target's
deps list in the BUILD.bazel file, with merge conflict markers (<<<<<<, =======,
>>>>>>>) that are invalid Bazel syntax. Remove all three conflict marker lines
and replace them with both required dependencies: include both
"//br/pkg/operation" and "//br/pkg/storage" in the deps list, as both are needed
by the test files (stream_metas_test.go, search_test.go, and
stream_misc_test.go). Ensure the dependencies are properly formatted as separate
entries in the deps array.
In `@br/pkg/stream/stream_metas_test.go`:
- Around line 27-31: The import section in the test file contains unresolved
merge conflict markers that prevent compilation. Remove the conflict markers
(<<<<<<< HEAD, =======, and >>>>>>> 807326b) and keep both import statements:
include both github.com/pingcap/tidb/br/pkg/storage and
github.com/pingcap/tidb/br/pkg/operation as separate import statements in the
imports block, since both packages are required by the test code.
- Line 2860: The assertion on mg.Warnings[0] uses require.Error incorrectly; the
string argument is treated as the test failure message, not as a check for
message content. Replace require.Error with require.ErrorContains to properly
verify that the warning message contains "this disk remembers nothing". The
correct usage of require.ErrorContains will validate both that mg.Warnings[0] is
a non-nil error and that its message contains the expected string.
In `@br/pkg/stream/stream_metas.go`:
- Around line 31-36: Resolve the Go merge conflict markers present throughout
the file by retaining both the storage import and the new operation and
backupmetas imports from the conflict sections. Locate the lockForAppend
function and update the error-path return statements that currently return
storage.RemoteLock{} to instead return objstore.RemoteLock{}, ensuring all
objstore types are properly imported and that the return type consistently uses
objstore.RemoteLock across the error paths in the function.
In `@br/pkg/task/BUILD.bazel`:
- Around line 135-139: The BUILD file contains unresolved merge conflict markers
around the deps list in the task_test target. Remove the conflict markers (the
lines starting with `<<<<<<<`, `=======`, and `>>>>>>>`), and keep both sets of
dependencies by retaining the existing deps from HEAD along with the newly added
deps for `//br/pkg/operation` and `//br/pkg/restore/log_client` from the
incoming commit. The reconciled deps list should include all dependencies
without any conflict markers.
In `@br/pkg/task/common_test.go`:
- Around line 14-20: The import block in the common_test.go file contains
unresolved merge conflict markers (<<<<<<< HEAD, =======, >>>>>>>). Remove all
conflict markers and consolidate the imports from both branches by keeping all
four import paths (the storage and utils imports from HEAD, plus the gc and
operation imports from the merged branch) in the proper import statement block.
Ensure the final import block includes all necessary packages without any merge
markers remaining.
In `@br/pkg/task/common.go`:
- Around line 30-34: Resolve the merge conflict in the import block of the
common.go file by removing the conflict markers (<<<<<<< HEAD, =======, and
>>>>>>> 807326b066f). Determine whether both the storage and operation imports
are needed for the code to function correctly, then keep the appropriate import
statement(s). If both imports are required by the code in this file, include
both import lines in the final version; if only one is needed, keep that single
import line.
In `@br/pkg/task/operator/migrate_to.go`:
- Around line 10-14: The file contains unresolved merge conflict markers
(<<<<<<< HEAD, =======, >>>>>>> markers) in three locations that must be
resolved before the code can compile. Examine the imports at lines 10-14, 60-65,
and 94-103 to determine which imports are actually required by the code in the
file. For each conflicting import, keep only the necessary import statements and
remove all merge conflict markers and the competing alternatives. If both the
storage and operation imports are needed, include both without conflict markers.
In `@br/pkg/task/stream.go`:
- Around line 1122-1127: Remove the merge conflict markers from the truncate
lock acquisition code around the storage lock operation. The conflict shows an
old approach using storage.TryLockRemote versus a new approach using
cfg.OperationContext.LockMeta with operation.LockResourceLogTruncateExclusive.
Based on the commit message indicating operation metadata was added to external
storage locks, keep the newer version that assigns to truncateLockMeta and calls
cfg.OperationContext.LockMeta, and remove all conflict markers (the <<<<<<<<,
======, and >>>>>>>> lines) so the code is syntactically valid.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: d64daf96-ce57-42e3-83e2-69d01c9ff24d
📒 Files selected for processing (23)
br/pkg/operation/BUILD.bazelbr/pkg/operation/context.gobr/pkg/operation/context_test.gobr/pkg/restore/log_client/BUILD.bazelbr/pkg/restore/log_client/client.gobr/pkg/restore/log_client/client_test.gobr/pkg/restore/snap_client/BUILD.bazelbr/pkg/restore/snap_client/pitr_collector.gobr/pkg/restore/snap_client/pitr_collector_test.gobr/pkg/storage/BUILD.bazelbr/pkg/storage/locking.gobr/pkg/storage/locking_test.gobr/pkg/storage/s3_test.gobr/pkg/stream/BUILD.bazelbr/pkg/stream/stream_metas.gobr/pkg/stream/stream_metas_test.gobr/pkg/task/BUILD.bazelbr/pkg/task/common.gobr/pkg/task/common_test.gobr/pkg/task/operator/BUILD.bazelbr/pkg/task/operator/migrate_to.gobr/pkg/task/restore.gobr/pkg/task/stream.go
Comment on lines
+135
to
+139
| <<<<<<< HEAD | ||
| ======= | ||
| "//br/pkg/operation", | ||
| "//br/pkg/restore/log_client", | ||
| >>>>>>> 807326b066f (br, pkg/objstore: add operation metadata to external storage locks (#69231)) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win
Resolve merge-conflict markers in task_test deps.
These conflict markers make the BUILD file invalid, so Bazel parsing/build is blocked until they are removed and the deps list is reconciled.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@br/pkg/task/BUILD.bazel` around lines 135 - 139, The BUILD file contains
unresolved merge conflict markers around the deps list in the task_test target.
Remove the conflict markers (the lines starting with `<<<<<<<`, `=======`, and
`>>>>>>>`), and keep both sets of dependencies by retaining the existing deps
from HEAD along with the newly added deps for `//br/pkg/operation` and
`//br/pkg/restore/log_client` from the incoming commit. The reconciled deps list
should include all dependencies without any conflict markers.
Comment on lines
+14
to
+20
| <<<<<<< HEAD | ||
| "github.com/pingcap/tidb/br/pkg/storage" | ||
| "github.com/pingcap/tidb/br/pkg/utils" | ||
| ======= | ||
| "github.com/pingcap/tidb/br/pkg/gc" | ||
| "github.com/pingcap/tidb/br/pkg/operation" | ||
| >>>>>>> 807326b066f (br, pkg/objstore: add operation metadata to external storage locks (#69231)) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win
Resolve merge-conflict markers in the test import block.
The file cannot compile with unresolved <<<<<<< / ======= / >>>>>>> markers.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@br/pkg/task/common_test.go` around lines 14 - 20, The import block in the
common_test.go file contains unresolved merge conflict markers (<<<<<<< HEAD,
=======, >>>>>>>). Remove all conflict markers and consolidate the imports from
both branches by keeping all four import paths (the storage and utils imports
from HEAD, plus the gc and operation imports from the merged branch) in the
proper import statement block. Ensure the final import block includes all
necessary packages without any merge markers remaining.
Contributor
|
/unhold Resolved the release-8.5 cherry-pick conflicts and pushed commit Summary:
Validation:
|
ti-chi-bot
Bot
removed
the
do-not-merge/hold
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## release-8.5 #69388 +/- ##
================================================
Coverage ? 55.6405%
================================================
Files ? 1849
Lines ? 666243
Branches ? 0
================================================
Hits ? 370701
Misses ? 267964
Partials ? 27578
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
YuJuncen
approved these changes
Jun 26, 2026
ti-chi-bot
Bot
added
approved
needs-1-more-lgtm
Leavrth
approved these changes
Jun 26, 2026
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: Leavrth, YuJuncen The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
ti-chi-bot
Bot
added
lgtm
and removed
needs-1-more-lgtm
Contributor
|
/retest |
1 similar comment
Contributor
|
/retest |
ti-chi-bot
Bot
added
cherry-pick-approved
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
4 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.
This is an automated cherry-pick of #69231
What problem does this PR solve?
Issue Number: ref #67819
Problem Summary:
BR external-storage lock files are permanent until removed. When a BR process crashes or gets stuck, operators need enough metadata in the lock file and conflict logs to identify which BR execution created or is blocked by a lock.
What changed and how does it work?
This PR adds operation-aware metadata for BR-created external-storage locks:
pkg/objstorechanges to the release branch'sbr/pkg/storagepackage layout.br/pkg/storage, includingoperation_id,operation_started_at,restore_id, andresource_type.br/pkg/operationto generate one command-scoped operation ID, log it at command start, attach restore ID when available, and build validated lock metadata.operator migrate-to.Check List
Tests
Manual test commands:
Side effects
Documentation
Release note
Please refer to Release Notes Language Style Guide to write a quality release note.
Summary by CodeRabbit
New Features
operation_started_atand restore-id hints on migration/lock objects.Refactor
Tests
Chores