Repository navigation
retry: delegate killed checks to signal handler - #2042
Conversation
ti-chi-bot
Bot
added
dco-signoff: yes
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthrough
ChangesKill signal handling
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant TransactionAction
participant Backoffer
participant Variables
participant KillSignalHandler
TransactionAction->>Backoffer: CheckKilled()
Backoffer->>Variables: LoadKillSignalHandler()
Variables-->>Backoffer: configured handler or nil
Backoffer->>KillSignalHandler: HandleSignal()
KillSignalHandler-->>Backoffer: error or nil
Backoffer-->>TransactionAction: interruption result
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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: 2
🤖 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 `@kv/variables.go`:
- Around line 39-40: Document the exported HandleSignal method in the
KillSignalHandler interface with a clear Go doc comment describing when it
returns an error, following standard Go documentation conventions.
In `@txnkv/transaction/2pc.go`:
- Around line 1103-1105: Extend transaction-level tests in 2pc_test.go beyond
TestMinCommitTsManager to cover doActionOnBatches with an interruptible batch
and a backoffer configured with a KillSignalHandler that returns
ErrQueryInterrupted. Assert that doActionOnBatches returns that interruption
error before invoking batch handling.
🪄 Autofix
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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 285cb786-9601-4402-bc04-5b5820e4703a
📒 Files selected for processing (4)
config/retry/backoff.goconfig/retry/backoff_test.gokv/variables.gotxnkv/transaction/2pc.go
da5feb9 to
416c4d8
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@kv/variables.go`:
- Around line 58-66: Synchronize access to Variables.KillSignalHandler between
SetKillSignalHandler and Backoffer.CheckKilled to eliminate races when shared
Variables are reused. Treat handler configuration as lifecycle state and either
protect both reads and writes with the existing synchronization mechanism or
store and retrieve the handler through a concurrency-safe copy/source, while
preserving the current handler precedence behavior.
🪄 Autofix
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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 52f9cd7b-82ca-4a37-afcd-fb1192fa5e49
📒 Files selected for processing (4)
config/retry/backoff.goconfig/retry/backoff_test.gokv/variables.gotxnkv/transaction/2pc.go
🚧 Files skipped from review as they are similar to previous changes (3)
- config/retry/backoff.go
- txnkv/transaction/2pc.go
- config/retry/backoff_test.go
|
/test pull-integration-test-nextgen |
0c306f8 to
bd0b65f
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
ti-chi-bot
Bot
added
size/L
bd0b65f to
1a233db
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
|
/test pull-integration-test-nextgen |
Signed-off-by: Yang Keao <yangkeao@chunibyo.icu>
1a233db to
764e536
Compare
| // SetKillSignalHandler atomically sets the handler used at interruptible KV request | ||
| // checkpoints. Passing nil clears the handler. | ||
| func (v *Variables) SetKillSignalHandler(handler interface{ HandleSignal() error }) { | ||
| if handler == nil { |
There was a problem hiding this comment.
Nit: we can also check if it's a typed nil
There was a problem hiding this comment.
After discuss with @lcwangchao, as we don't pass any typed nil here yet, it's too redudant to handle the case. I'll remove the typed nil check based on reflect.
Signed-off-by: Yang Keao <yangkeao@chunibyo.icu>
ti-chi-bot
Bot
added
needs-1-more-lgtm
| return v.killSignalHandler | ||
| } | ||
|
|
||
| func isNilKillSignalHandler(handler KillSignalHandler) bool { |
There was a problem hiding this comment.
Why does it need to introduce isNilKillSignalHandler instead of handler == nil ? Does this have any special case to make us to have to do this?
There was a problem hiding this comment.
No. For the current use case == nil is enough. But I received a previews comment:
And I think it would be better than not 🤦. As this branch will not be called very frequently, it'll not introduce high performance influence.
Signed-off-by: Yang Keao <yangkeao@chunibyo.icu>
ti-chi-bot
Bot
removed
the
size/L
ti-chi-bot
Bot
added
the
size/M
Signed-off-by: Yang Keao <yangkeao@chunibyo.icu>
| if b.vars == nil { | ||
| return nil | ||
| } | ||
| if handler := b.vars.LoadKillSignalHandler(); handler != nil { |
There was a problem hiding this comment.
Do we need to aslo check b.vars.Killed here ? Is there any case we need to ignore b.vars.Killed ?
There was a problem hiding this comment.
No. I will move it to the end! 👍
Signed-off-by: Yang Keao <yangkeao@chunibyo.icu>
92986cf to
caafcfe
Compare
Signed-off-by: Yang Keao <yangkeao@chunibyo.icu>
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: ekexium, lcwangchao 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 |
[LGTM Timeline notifier]Timeline:
|
\n\nSigned-off-by: Yang Keao <yangkeao@chunibyo.icu> Signed-off-by: Yang Keao <yangkeao@chunibyo.icu>
Signed-off-by: Yang Keao <yangkeao@chunibyo.icu>
\n\nSigned-off-by: Yang Keao <yangkeao@chunibyo.icu> (cherry picked from commit 52c1e76) Signed-off-by: Yang Keao <yangkeao@chunibyo.icu>
3 participants
Problem
TiDB needs to run richer kill-signal handling at interruptible TiKV retry checkpoints so it can cooperatively detect a disconnected client while a statement is blocked in TiKV. The existing client-go code only reads the legacy
Killedflag, and the 2PC path duplicates that check.This is needed by pingcap/tidb#68682.
Downstream integration: pingcap/tidb#70343.
What changed
kv.KillSignalHandlerinterface tokv.Variables.Backoffer.CheckKilleddelegate to the handler when configured, while preserving the existingKilledfallback for compatibility.Backoffer.CheckKilledfor interruptible 2PC actions instead of duplicating the atomic kill-flag check.Compatibility
This is an additive API change. Existing callers that only populate
Variables.Killedkeep the previous behavior. TiDB can opt in by installing its SQL kill-signal handler.Tests
go test ./config/retry ./txnkv/transactionSummary by CodeRabbit
New Features
Bug Fixes