txnkv: Support resolve txn file locks#2003
Conversation
Signed-off-by: Ping Yu <yuping@pingcap.com>
Signed-off-by: Ping Yu <yuping@pingcap.com>
📝 WalkthroughWalkthrough
ChangesIsTxnFile Propagation Through Lock Resolution
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
txnkv/txnlock/lock_resolver.go (1)
900-907:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftKey the resolved-status cache by
IsTxnFiletoo.
getTxnStatusnow builds differentCheckTxnStatusRequestvariants, but Line 1003 still short-circuits on a cache keyed only bytxnID, andGetTxnStatuson Lines 900-907 can only populate that cache through thelockInfo == nil/IsTxnFile=falsepath. A prior lookup on the non-file path can therefore suppress the correctly flagged RPC for a txn-file lock (and vice versa), which risks resolving the transaction with a status produced under different server semantics.Also applies to: 1001-1031
🤖 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 `@txnkv/txnlock/lock_resolver.go` around lines 900 - 907, The resolved-status cache is currently keyed only by txnID, causing mismatches between file vs non-file RPC variants; update the cache key and all cache lookups/writes in getTxnStatus (and the path used by GetTxnStatus) to include the IsTxnFile flag so entries distinguish CheckTxnStatusRequest variants; ensure any early-return that uses lockInfo==nil/IsTxnFile=false and the code paths around the CheckTxnStatusRequest construction (including the region around lines 1001-1031) both read and populate the cache with the combined (txnID, IsTxnFile) key so a prior lookup on one variant cannot suppress a correct RPC for the other.
🧹 Nitpick comments (1)
txnkv/txnlock/lock_resolver.go (1)
200-212: ⚡ Quick winAdd a doc comment for
Lock.IsTxnFile.
Lockis part of the publictxnkvsurface, so the new exported field should explain what “txn file” means and when callers can expect it to be set.As per coding guidelines, "Exported identifiers must have clear Go doc comments when they are part of the public client API".
🤖 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 `@txnkv/txnlock/lock_resolver.go` around lines 200 - 212, Add a Go doc comment for the exported field Lock.IsTxnFile on the Lock struct that clearly explains what a "txn file" is and when the flag is set; update the comment above the Lock.IsTxnFile field (reference Lock.IsTxnFile) to describe its semantic meaning (e.g., indicates the lock was created by a transaction using a transaction-file based commit path or metadata persisted to a txn file), under what conditions callers will see it true (e.g., specific commit/transaction modes or servers that set this flag), and any implications for callers (e.g., how it changes lock resolution behavior or expectations).Source: Coding guidelines
🤖 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.
Outside diff comments:
In `@txnkv/txnlock/lock_resolver.go`:
- Around line 900-907: The resolved-status cache is currently keyed only by
txnID, causing mismatches between file vs non-file RPC variants; update the
cache key and all cache lookups/writes in getTxnStatus (and the path used by
GetTxnStatus) to include the IsTxnFile flag so entries distinguish
CheckTxnStatusRequest variants; ensure any early-return that uses
lockInfo==nil/IsTxnFile=false and the code paths around the
CheckTxnStatusRequest construction (including the region around lines 1001-1031)
both read and populate the cache with the combined (txnID, IsTxnFile) key so a
prior lookup on one variant cannot suppress a correct RPC for the other.
---
Nitpick comments:
In `@txnkv/txnlock/lock_resolver.go`:
- Around line 200-212: Add a Go doc comment for the exported field
Lock.IsTxnFile on the Lock struct that clearly explains what a "txn file" is and
when the flag is set; update the comment above the Lock.IsTxnFile field
(reference Lock.IsTxnFile) to describe its semantic meaning (e.g., indicates the
lock was created by a transaction using a transaction-file based commit path or
metadata persisted to a txn file), under what conditions callers will see it
true (e.g., specific commit/transaction modes or servers that set this flag),
and any implications for callers (e.g., how it changes lock resolution behavior
or expectations).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 40ab777f-fa4f-4d52-a6d3-4eced86d80d4
📒 Files selected for processing (3)
internal/client/client_collapse.gointernal/client/client_test.gotxnkv/txnlock/lock_resolver.go
|
/retest |
1 similar comment
|
/retest |
Signed-off-by: Ping Yu <yuping@pingcap.com>
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: coocood The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
[LGTM Timeline notifier]Timeline:
|
|
@pingyu: The following test failed, say
Full PR test history. Your PR dashboard. 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 kubernetes-sigs/prow repository. I understand the commands that are listed here. |
Summary
Pick resolve locks part from #1119.
Resolve locks logics only, to be used for components (e.g. TiCDC) which will try to resolve locks.
Should have no impact when the txn file feature is disabled and TiDB do not generate txn file transactions.
The full txn file transaction feature will be in another PR (see #1998).
Tests
Summary by CodeRabbit