ingest: use 2 level split & scatter#66354
Conversation
Signed-off-by: ti-chi-bot <ti-community-prow-bot@tidb.io>
ti-chi-bot
added
do-not-merge/hold
|
@D3Hunter This PR has conflicts, I have hold it. |
|
I've accepted your request and will start reviewing the pull request. I'll conduct a thorough review covering code quality, potential issues, and implementation details. ⏳ This process typically takes 10-30 minutes depending on the complexity of the changes.
|
|
@ti-chi-bot: ## If you want to know how to resolve it, please read the guide in TiDB Dev Guide. 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 ti-community-infra/tichi repository. |
![pantheon-ai[bot]](https://avatars.githubusercontent.com/in/2265341?s=60&v=4){kind=small}
|
@pantheon-ai[bot]: adding LGTM is restricted to approvers and reviewers in OWNERS files. 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. |
D3Hunter
changed the title
D3Hunter
changed the title
|
Review of updated commits could not be completed at this time due to a temporary infrastructure outage (execution providers unavailable). Please re-trigger the review once the platform recovers. ℹ️ Learn more details on Pantheon AI. |
📝 WalkthroughWalkthroughAdds a two-level splitting path: for large split-key sets, compute a coarse-grained subset (~√n), scatter those keys first, then continue batched splitting with rate limiting; includes helper functions and tests. No public API changes. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). 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 |
…o-level split/scatter tests and coarse threshold
|
Review of updated commits (dea115a) could not be completed — execution infrastructure is temporarily unavailable (CR2 provider overloaded). Please re-trigger the review once the platform recovers. ℹ️ 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.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/lightning/backend/local/localhelper_test.go (1)
374-378:⚠️ Potential issue | 🔴 CriticalAdd missing
}to closeTestTuneStoreWriteLimiterfunction.
TestTuneStoreWriteLimiterstarting at line 343 is missing its closing brace. Line 378 beginsTestSplitAndScatterRegionInBatchesTwoLevelwhile the previous function block remains unclosed, causing a compilation error.🐛 Proposed fix
limiter.UpdateLimit(200) ctx1, cancel1 := context.WithTimeout(context.Background(), time.Second*2) defer cancel1() testLimiter(ctx1, 200) +} + func TestSplitAndScatterRegionInBatchesTwoLevel(t *testing.T) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/lightning/backend/local/localhelper_test.go` around lines 374 - 378, The test function TestTuneStoreWriteLimiter is missing its closing brace; add a single `}` to terminate TestTuneStoreWriteLimiter (the function that calls limiter.UpdateLimit(200), context.WithTimeout and testLimiter(ctx1, 200)) so the subsequent TestSplitAndScatterRegionInBatchesTwoLevel function is not nested and the file compiles.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@pkg/lightning/backend/local/localhelper_test.go`:
- Around line 374-378: The test function TestTuneStoreWriteLimiter is missing
its closing brace; add a single `}` to terminate TestTuneStoreWriteLimiter (the
function that calls limiter.UpdateLimit(200), context.WithTimeout and
testLimiter(ctx1, 200)) so the subsequent
TestSplitAndScatterRegionInBatchesTwoLevel function is not nested and the file
compiles.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 959632cc-cd27-49af-94c3-81ebd373f009
📒 Files selected for processing (1)
pkg/lightning/backend/local/localhelper_test.go
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #66354 +/- ##
================================================
+ Coverage 77.6724% 78.2835% +0.6110%
================================================
Files 2006 1939 -67
Lines 548699 544168 -4531
================================================
- Hits 426188 425994 -194
+ Misses 120851 117722 -3129
+ Partials 1660 452 -1208
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
|
Review Complete Findings: 0 issues ℹ️ Learn more details on Pantheon AI. |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
pkg/lightning/backend/local/localhelper_test.go (1)
381-387: Extract duplicatedmakeSplitKeyshelper to file scope.The same helper is declared twice; pulling it into one file-level helper will keep future test edits centralized.
♻️ Proposed refactor
+func makeSplitKeys(n int) [][]byte { + keys := make([][]byte, n) + for i := 0; i < n; i++ { + keys[i] = []byte{byte(i >> 8), byte(i)} + } + return keys +} + func TestSplitAndScatterRegionInBatchesTwoLevel(t *testing.T) { - makeSplitKeys := func(n int) [][]byte { - keys := make([][]byte, n) - for i := 0; i < n; i++ { - keys[i] = []byte{byte(i >> 8), byte(i)} - } - return keys - } - t.Run("large split keys trigger coarse and fine layers", func(t *testing.T) { ... }) } func TestGetCoarseGrainedSplitKeys(t *testing.T) { - makeSplitKeys := func(n int) [][]byte { - keys := make([][]byte, n) - for i := 0; i < n; i++ { - keys[i] = []byte{byte(i >> 8), byte(i)} - } - return keys - } - t.Run("last key selected in loop is not appended twice", func(t *testing.T) { ... }) }Also applies to: 443-449
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/lightning/backend/local/localhelper_test.go` around lines 381 - 387, Extract the duplicated makeSplitKeys helper into a single file-scoped function named makeSplitKeys in localhelper_test.go and remove the other duplicate declarations; specifically, replace the two local copies (the one shown and the separate copy around lines 443-449) with one top-level function so tests reuse the same helper, keeping the function signature makeSplitKeys(n int) [][]byte and its existing implementation that builds keys with []byte{byte(i >> 8), byte(i)}.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@pkg/lightning/backend/local/localhelper_test.go`:
- Around line 381-387: Extract the duplicated makeSplitKeys helper into a single
file-scoped function named makeSplitKeys in localhelper_test.go and remove the
other duplicate declarations; specifically, replace the two local copies (the
one shown and the separate copy around lines 443-449) with one top-level
function so tests reuse the same helper, keeping the function signature
makeSplitKeys(n int) [][]byte and its existing implementation that builds keys
with []byte{byte(i >> 8), byte(i)}.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 0545410e-10f8-4ad3-a6b4-7a3e8657e85b
📒 Files selected for processing (1)
pkg/lightning/backend/local/localhelper_test.go
|
/retest |
ti-chi-bot
bot
added
approved
needs-1-more-lgtm
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: D3Hunter, OliverS929 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
[LGTM Timeline notifier]Timeline:
|
|
/unhold |
ti-chi-bot
bot
removed
do-not-merge/hold
3 participants
This is an automated cherry-pick of #66312
### What problem does this PR solve?
Issue Number: ref #66311
Problem Summary:
NOTE: master doesn't have this feature, so there is no
RESTOREbelow is the original description
#62419introduced split/ingest rate limiting but removed the coarse split/scatter stage.What changed and how does it work?
pkg/lightning/backend/local/localhelper.go:len(splitKeys) > 100#62419limiter behavior and apply it to both levels.pkg/lightning/backend/local/localhelper_test.go:Check List
Tests
Side effects
Documentation
Release note
Summary by CodeRabbit
Refactor
Tests