pkg/lightning, tests/realtikvtest: stabilize next-gen add-index tests#67831
pkg/lightning, tests/realtikvtest: stabilize next-gen add-index tests#67831ti-chi-bot[bot] merged 3 commits intopingcap:masterfrom
Conversation
|
Skipping CI for Draft Pull Request. |
ti-chi-bot
Bot
added
release-note-none
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (7)
🚧 Files skipped from review as they are similar to previous changes (6)
📝 WalkthroughWalkthroughAdds failpoint hooks to force split-before-ingest and to let tests override region-job retry backoff; applies these hooks in multiple add-index realtikv tests and updates two Bazel test targets. Changes
Sequence Diagram(s)sequenceDiagram
participant Dispatcher as Dispatcher
participant Failpoint as Failpoint
participant Time as Time
participant Retryer as Retryer
Dispatcher->>Failpoint: InjectCall("adjustRegionJobRetryBackoff", &backoff)
Failpoint-->>Dispatcher: (maybe modify) backoff
Dispatcher->>Time: Now().Add(backoff)
Time-->>Dispatcher: waitUntil timestamp
Dispatcher->>Retryer: push(job) when retryable
Retryer-->>Dispatcher: job requeued
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 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)
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 |
ti-chi-bot
Bot
added
the
size/M
|
Hi @D3Hunter. 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. |
ti-chi-bot
Bot
removed
the
do-not-merge/work-in-progress
|
Review Complete Findings: 0 issues ℹ️ Learn more details on Pantheon AI. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/realtikvtest/addindextest/failpoints_test.go (1)
24-27: Move failpoint setup afterFullModeguards for skipped paths.At Line 24, Line 38, and Line 70,
alwaysSplitAndReduceBackoff(t)runs even when the test immediately skips. For consistency with Line 55 and Line 64 (and to keep skipped paths minimal), place the helper after theif !*FullMode { t.Skip() }guard.♻️ Suggested refactor
func TestFailpointsCreateNonUniqueIndex(t *testing.T) { - alwaysSplitAndReduceBackoff(t) if !*FullMode { t.Skip() } + alwaysSplitAndReduceBackoff(t) var colIDs = [][]int{ @@ func TestFailpointsCreateUniqueIndex(t *testing.T) { - alwaysSplitAndReduceBackoff(t) if !*FullMode { t.Skip() } + alwaysSplitAndReduceBackoff(t) var colIDs = [][]int{ @@ func TestFailpointsCreateMultiColsIndex(t *testing.T) { - alwaysSplitAndReduceBackoff(t) if !*FullMode { t.Skip() } + alwaysSplitAndReduceBackoff(t) var coliIDs = [][]int{As per coding guidelines: "Keep test changes minimal and deterministic; avoid broad golden/testdata churn unless required."
Also applies to: 38-41, 70-73
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/realtikvtest/addindextest/failpoints_test.go` around lines 24 - 27, The call to alwaysSplitAndReduceBackoff(t) is being executed before the FullMode guard, causing unnecessary failpoint setup even when the test immediately calls t.Skip(); move the alwaysSplitAndReduceBackoff(t) invocation to after the if !*FullMode { t.Skip() } check in each affected test so the failpoint/setup runs only for FullMode runs; update the three occurrences that currently precede the FullMode guard so they follow the guard, leaving other behavior unchanged (references: alwaysSplitAndReduceBackoff, FullMode, t.Skip).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@tests/realtikvtest/addindextest/failpoints_test.go`:
- Around line 24-27: The call to alwaysSplitAndReduceBackoff(t) is being
executed before the FullMode guard, causing unnecessary failpoint setup even
when the test immediately calls t.Skip(); move the
alwaysSplitAndReduceBackoff(t) invocation to after the if !*FullMode { t.Skip()
} check in each affected test so the failpoint/setup runs only for FullMode
runs; update the three occurrences that currently precede the FullMode guard so
they follow the guard, leaving other behavior unchanged (references:
alwaysSplitAndReduceBackoff, FullMode, t.Skip).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 7fdffcd5-aba1-46b1-8fcb-9e4c2fa315e0
📒 Files selected for processing (9)
br/pkg/metautil/BUILD.bazelpkg/importsdk/BUILD.bazelpkg/lightning/backend/local/local.gopkg/lightning/backend/local/region_job.gotests/realtikvtest/addindextest/add_index_test.gotests/realtikvtest/addindextest/concurrent_ddl_test.gotests/realtikvtest/addindextest/failpoints_test.gotests/realtikvtest/addindextest/multi_schema_change_test.gotests/realtikvtest/addindextest/pitr_test.go
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #67831 +/- ##
================================================
+ Coverage 77.5964% 79.3266% +1.7302%
================================================
Files 1982 1993 +11
Lines 548885 549339 +454
================================================
+ Hits 425915 435772 +9857
+ Misses 122165 112121 -10044
- Partials 805 1446 +641
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
|
/retest |
|
@D3Hunter: PRs from untrusted users cannot be marked as trusted with DetailsIn response to this:
Instructions 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. |
|
🔍 Starting code review for this PR... |
ingress-bot
left a comment
ingress-bot
left a comment
There was a problem hiding this comment.
This review was generated by AI and should be verified by a human reviewer.
Manual follow-up is recommended before merge.
Summary
- Total findings: 2
- Inline comments: 2
- Summary-only findings (no inline anchor): 0
Findings (highest risk first)
🟡 [Minor] (1)
- Workaround helper name/comment do not state the altered test-path contract (tests/realtikvtest/addindextest/add_index_test.go:39, tests/realtikvtest/addindextest/concurrent_ddl_test.go:26, tests/realtikvtest/addindextest/failpoints_test.go:24, tests/realtikvtest/addindextest/multi_schema_change_test.go:24, tests/realtikvtest/addindextest/pitr_test.go:24)
🧹 [Nit] (1)
- Failpoint callback parameter name hides
time.Durationsemantics (tests/realtikvtest/addindextest/add_index_test.go:50, pkg/lightning/backend/local/region_job.go:1039)
|
/retest |
|
@D3Hunter: PRs from untrusted users cannot be marked as trusted with DetailsIn response to this:
Instructions 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. |
ti-chi-bot
Bot
added
lgtm
and removed
needs-1-more-lgtm
[LGTM Timeline notifier]Timeline:
|
|
/retest |
|
@D3Hunter: PRs from untrusted users cannot be marked as trusted with DetailsIn response to this:
Instructions 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. |
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: D3Hunter, lance6716, Leavrth, wjhuang2016 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 |
|
/retest |
|
@D3Hunter: PRs from untrusted users cannot be marked as trusted with DetailsIn response to this:
Instructions 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. |
|
/retest |
1 similar comment
|
/retest |
|
@D3Hunter: PRs from untrusted users cannot be marked as trusted with DetailsIn response to this:
Instructions 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. |
|
/retest |
|
@D3Hunter: PRs from untrusted users cannot be marked as trusted with DetailsIn response to this:
Instructions 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. |
![pantheon-ai[bot]](https://avatars.githubusercontent.com/in/2265341?s=60&v=4){kind=small}
6 participants
What problem does this PR solve?
Issue Number: close #67830
Problem Summary:
Next-gen
tests/realtikvtest/addindextestcases can spend most of their runtime on split/ingest retry churn and hit the shard timeout budget. The tests need a reliable way to force split-before-ingest and to tune the region-job retry backoff used in the retry path.What changed and how does it work?
adjustNeedSplitfailpoint hook in the local backend so tests can force split-before-ingestadjustRegionJobRetryBackoffto override atime.Durationdirectly, so tests can also use sub-second backoff values when neededalwaysSplitAndReduceBackoffhelper intests/realtikvtest/addindextestand apply it to the next-gen add-index suitesMeasured result on
TestCreateNonUniqueIndex:276.38stest time /279.570stotal64.19stest time /67.432stotalCheck List
Tests
Manual test steps:
make failpoint-enable go test -run TestCreateNonUniqueIndex -tags=intest,deadlock,nextgen -count=1 -v ./tests/realtikvtest/addindextest make failpoint-disableSide effects
Documentation
Release note
Please refer to Release Notes Language Style Guide to write a quality release note.
Summary by CodeRabbit