Skip to content

executor: reduce TestDistSQLSharedKVRequestRace iterations to fix CI timeout #67675

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
joechenrh wants to merge 3 commits into
base: master
Choose a base branch
from
+4 −1
Open

executor: reduce TestDistSQLSharedKVRequestRace iterations to fix CI timeout #67675

Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
e70180b
executor: reduce TestDistSQLSharedKVRequestRace iterations to fix CI …
joechenrh Apr 10, 2026
379949c
executor: add comment explaining iteration count rationale
joechenrh Apr 10, 2026
39c3ec6
executor: use for range idiom
joechenrh Apr 10, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion pkg/executor/test/distsqltest/distsql_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -135,7 +135,7 @@ func TestDistSQLSharedKVRequestRace(t *testing.T) {
}
for _, mode := range replicaReadModes {
tk.MustExec(fmt.Sprintf("set session tidb_replica_read = '%s'", mode))
for i := 0; i < 20; i++ {
for i := 0; i < 5; i++ {
Copy link
Copy Markdown

@ingress-bot ingress-bot Apr 10, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 [Minor] Race regression test lost replay coverage after loop-count reduction

Impact
TestDistSQLSharedKVRequestRace now runs each replica-read mode 5 times instead of 20, reducing stress executions from 200 to 50 across the two query paths.
This reduces replay/retry sampling for a schedule-sensitive race regression, so repeated runs no longer provide the prior detection confidence.

Scope

  • pkg/executor/test/distsqltest/distsql_test.go:138 — TestDistSQLSharedKVRequestRace

Evidence
The changed loop bound is for i := 0; i < 5; i++, replacing the previous 20-iteration stress loop in TestDistSQLSharedKVRequestRace.
That loop wraps both force index(ic) and index-merge query checks for every tidb_replica_read mode, so each mode now executes only one quarter of the previous repetition count.

Change request
Restore the stress loop count to the previous level, or introduce an explicit deterministic stress knob with documented rationale for lower coverage.
Keep per-mode repeated executions high enough that race detection remains stable across reruns and scheduler variance.

Copy link
Copy Markdown

@ingress-bot ingress-bot Apr 10, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 [Minor] Race reproducer coverage is reduced by cutting loop repetitions

Impact
TestDistSQLSharedKVRequestRace is the regression guard for shared kv.Request race behavior from issue 60175, and this patch cuts repeated executions from 20 to 5 per replica-read mode.
The reduced repetition lowers scheduler interleaving coverage, allowing low-frequency race regressions to pass this guard.

Scope

  • pkg/executor/test/distsqltest/distsql_test.go:138 — TestDistSQLSharedKVRequestRace

Evidence
In TestDistSQLSharedKVRequestRace, the inner loop now runs 5 iterations instead of 20 while executing the same two query paths each round.
The function comment marks this test as the regression check for https://github.com/pingcap/tidb/issues/60175, so reducing only the repetition count removes stress coverage without adding a deterministic trigger.

Change request
Restore the previous repetition budget or replace it with a deterministic concurrency trigger that guarantees the race window is exercised on every run.
If test time is the concern, keep a fast-path count here only with an additional stress variant that preserves equivalent race-detection strength in CI.

Copy link
Copy Markdown

@ingress-bot ingress-bot Apr 10, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 [Minor] Race-check loop bound changed without intent documentation

Impact
TestDistSQLSharedKVRequestRace is explicitly tied to issue 60175, but the new 5 iteration bound is an unexplained magic number in a stress-style test.
Without rationale for why this bound is sufficient, later edits can keep shrinking or reshaping the probe while the test name still implies strong race-coverage intent.

Scope

  • pkg/executor/test/distsqltest/distsql_test.go:138 — TestDistSQLSharedKVRequestRace

Evidence
The diff changes the inner loop from for i := 0; i < 20; i++ to for i := 0; i < 5; i++ at line 138.
The nearby comments only label query forms (index lookup and index merge) and do not document the expected repetition invariant or the tradeoff behind the new bound.

Change request
Introduce an intent-revealing constant name for this loop bound and add a short comment explaining why the chosen count is sufficient for the 60175 regression guard.
Document the accepted coverage/performance tradeoff at this line so future maintainers can adjust it without guesswork.

// index lookup
tk.MustQuery("select * from t force index(ic) order by c asc limit 500").Check(testkit.Rows(expects...))
// index merge
Expand Down
Loading