Repository navigation
txn: Add test for the issue that transaction using fair locking multiple times in the first statement may make keeping-alive fail - #67141
Conversation
Signed-off-by: MyonKeminta <MyonKeminta@users.noreply.github.com>
ti-chi-bot
Bot
added
release-note
|
Review Complete Findings: 1 issues ℹ️ Learn more details on Pantheon AI. |
ti-chi-bot
Bot
added
size/M
|
Hi @MyonKeminta. Thanks for your PR. PRs from untrusted users cannot be marked as trusted with I understand the commands that are listed here. 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. |
📝 WalkthroughWalkthroughAdded a new test function Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes 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)
📝 Coding Plan
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 Tip CodeRabbit can generate a title for your PR based on the changes with custom instructions.Set the |
|
/ok-to-test |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
tests/realtikvtest/pessimistictest/pessimistic_test.go (1)
3503-3561: Name the scenarios instead of passing booleans.
test(true, false)andtest(true, true)make the matrix hard to read, and when one branch fails the output will not say which scenario regressed. A small table-driven setup withnameplust.Runwould make the retry/primary-change cases much easier to understand and debug.As per coding guidelines, Code SHOULD be self-documenting through clear naming and structure; when naming alone may not make intent obvious, add a brief comment.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/realtikvtest/pessimistictest/pessimistic_test.go` around lines 3503 - 3561, The anonymous test closure taking (isRetried bool, isPrimaryChanged bool) makes outcomes opaque; refactor into a table-driven subtests using t.Run with descriptive names (e.g., "no-retry", "retry-no-primary-change", "retry-primary-changed") and a struct with fields Retried and PrimaryChanged, then iterate the table and call the existing test body (the logic inside the test closure) per entry; update references to isRetried/isPrimaryChanged inside the closure (or convert the closure to a helper func like runPessimisticTest) to use the struct fields, so failures print the subtest name and intent clearly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/realtikvtest/pessimistictest/pessimistic_test.go`:
- Around line 3507-3556: The test only exercises fair-locking via UPDATE but
misses the unique-key point-get regression path (SELECT ... FOR UPDATE) reported
in `#66571`; add a variant where session tk does a "SELECT ... FOR UPDATE" on uk =
10 as its first statement (using tk.MustQuery or tk.MustExec with "select * from
t where uk = 10 for update") before the conflicting operations, keeping the rest
of the flow (tk2 waits with update and hits ErrLockWaitTimeout, tk commits,
final check) and reuse the existing variables (additionalInitStmt,
earlyConflictingStmt, expectedFinalData) and session names (tk, tk2, tk3) so the
new branch follows the same setup/teardown and assertions as the other cases.
---
Nitpick comments:
In `@tests/realtikvtest/pessimistictest/pessimistic_test.go`:
- Around line 3503-3561: The anonymous test closure taking (isRetried bool,
isPrimaryChanged bool) makes outcomes opaque; refactor into a table-driven
subtests using t.Run with descriptive names (e.g., "no-retry",
"retry-no-primary-change", "retry-primary-changed") and a struct with fields
Retried and PrimaryChanged, then iterate the table and call the existing test
body (the logic inside the test closure) per entry; update references to
isRetried/isPrimaryChanged inside the closure (or convert the closure to a
helper func like runPessimisticTest) to use the struct fields, so failures print
the subtest name and intent clearly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 3fa04150-3f55-47af-be6c-ba3a36e01a6f
📒 Files selected for processing (1)
tests/realtikvtest/pessimistictest/pessimistic_test.go
| additionalInitStmt := "" | ||
| earlyConflictingStmt := "" | ||
| testedStmt := "update t set v = v + 1 where uk = 10" | ||
| expectedFinalData := []string{"1 10 101"} | ||
|
|
||
| if isRetried { | ||
| if !isPrimaryChanged { | ||
| additionalInitStmt = "insert into t values (0, 0, 10)" | ||
| earlyConflictingStmt = "update t set v = 200 where uk = 10" | ||
| expectedFinalData = []string{"0 0 10", "1 10 201"} | ||
| } else { | ||
| additionalInitStmt = "insert into t values (0, 0, 9)" | ||
| earlyConflictingStmt = "update t set v = 10 where id = 0" | ||
| testedStmt = ` | ||
| with | ||
| x as (select /*+ MERGE() */ * from t where id = 0) | ||
| update x join t on x.v = t.uk set t.v = t.v + 1 where t.uk = 10` | ||
| expectedFinalData = []string{"0 0 10", "1 10 101"} | ||
| } | ||
| } | ||
|
|
||
| defer setLockTTL(100).restore() | ||
| tk.MustExec("set @@tidb_pessimistic_txn_fair_locking=1") | ||
| tk.MustExec("use test") | ||
| tk.MustExec("drop table if exists t") | ||
| tk.MustExec("create table t (id int primary key, uk int unique, v int)") | ||
| if additionalInitStmt != "" { | ||
| tk.MustExec(additionalInitStmt) | ||
| } | ||
| tk.MustExec("insert into t values (1, 10, 100)") | ||
| tk2.MustExec("use test") | ||
| tk2.MustExec("set innodb_lock_wait_timeout = 1") | ||
|
|
||
| tk.MustExec("begin pessimistic") | ||
| if earlyConflictingStmt != "" { | ||
| tk3 := testkit.NewTestKit(t, store) | ||
| tk3.MustExec("use test") | ||
| tk3.MustExec(earlyConflictingStmt) | ||
| } | ||
| tk.MustExec(testedStmt) | ||
|
|
||
| tk2.MustExec("begin pessimistic") | ||
| err := tk2.ExecToErr("update t set v = v + 2 where uk = 10") | ||
| require.Error(t, err) | ||
| require.ErrorIs(t, err, storeerr.ErrLockWaitTimeout) | ||
| tk2.MustExec("commit") | ||
|
|
||
| tk.MustExec("commit") | ||
|
|
||
| tk.MustQuery("select * from t").Check(testkit.Rows(expectedFinalData...)) |
There was a problem hiding this comment.
Please cover the original SELECT ... FOR UPDATE regression path directly.
All three variants exercise fair-locking through UPDATE, but #66571 is a unique-key point-get SELECT ... FOR UPDATE case. This file already treats pessimisticSelectForUpdateRetry and pessimisticDMLRetry as distinct paths, so a DML-only regression test can stay green while the reported FOR UPDATE behavior breaks again. Please add a case where session 1’s first statement is SELECT ... FOR UPDATE on uk = 10, and session 2 waits on the same key.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/realtikvtest/pessimistictest/pessimistic_test.go` around lines 3507 -
3556, The test only exercises fair-locking via UPDATE but misses the unique-key
point-get regression path (SELECT ... FOR UPDATE) reported in `#66571`; add a
variant where session tk does a "SELECT ... FOR UPDATE" on uk = 10 as its first
statement (using tk.MustQuery or tk.MustExec with "select * from t where uk = 10
for update") before the conflicting operations, keeping the rest of the flow
(tk2 waits with update and hits ErrLockWaitTimeout, tk commits, final check) and
reuse the existing variables (additionalInitStmt, earlyConflictingStmt,
expectedFinalData) and session names (tk, tk2, tk3) so the new branch follows
the same setup/teardown and assertions as the other cases.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #67141 +/- ##
================================================
+ Coverage 77.7082% 77.8920% +0.1837%
================================================
Files 2016 1942 -74
Lines 552461 548707 -3754
================================================
- Hits 429308 427399 -1909
+ Misses 121411 121296 -115
+ Partials 1742 12 -1730
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
![pantheon-ai[bot]](https://avatars.githubusercontent.com/in/2265341?s=60&v=4){kind=small}
| } | ||
| } | ||
|
|
||
| defer setLockTTL(100).restore() |
There was a problem hiding this comment.
[P2] Test flakiness risk from very short lock TTL (100ms) in realtikv test
Why: May intermittently fail in CI because keeping locks alive for a 1s wait requires frequent TTL heartbeats; small scheduling/RPC jitter can let locks expire and make the waiter acquire the lock instead of timing out.
Evidence: defer setLockTTL(100).restore() with tk2.MustExec("set innodb_lock_wait_timeout = 1") and require.ErrorIs(t, err, storeerr.ErrLockWaitTimeout) at tests/realtikvtest/pessimistictest/pessimistic_test.go:3528. The keepalive ticker period is ManagedLockTTL/2 (~50ms), and any transient scheduling/RPC stall can let the lock appear expired.
ti-chi-bot
Bot
added
approved
needs-1-more-lgtm
|
/retest |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: cfzjywxk, ekexium 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
MyonKeminta
added
the
needs-cherry-pick-release-8.5
|
In response to a cherrypick label: new pull request created to branch |
What problem does this PR solve?
Issue Number: close #66571
Problem Summary:
This PR adds test for the issue #66571 .
It includes updating client-go, but was already included by other PRs.
What changed and how does it work?
Check List
Tests
Side effects
Documentation
Release note
Please refer to Release Notes Language Style Guide to write a quality release note.
Summary by CodeRabbit