schedule/labeler: index keyspace rules incrementally#11031
Conversation
Signed-off-by: Ryan Leung <rleungx@gmail.com>
ti-chi-bot
Bot
added
release-note
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: 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 |
ti-chi-bot
Bot
added
the
size/XXL
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughRegionLabeler now separates generic and canonical keyspace rules, maintains a sparse deterministic keyspace index, combines both sources during label lookup, and merges their split keys. Tests and benchmarks cover boundaries, updates, compatibility, and performance. ChangesKeyspace rule indexing
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant RegionLabeler
participant rangeList
participant keyspaceRuleIndex
participant RegionLabel
RegionLabeler->>rangeList: query generic range data
RegionLabeler->>keyspaceRuleIndex: query canonical rule and split boundaries
rangeList-->>RegionLabeler: return generic rules
keyspaceRuleIndex-->>RegionLabeler: return keyspace rule
RegionLabeler->>RegionLabel: resolve highest-index labels
Possibly related issues
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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 |
|
/run-check-issue-triage-complete |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #11031 +/- ##
==========================================
+ Coverage 79.23% 79.27% +0.04%
==========================================
Files 541 542 +1
Lines 76035 76320 +285
==========================================
+ Hits 60245 60503 +258
- Misses 11545 11549 +4
- Partials 4245 4268 +23
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
![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)
pkg/schedule/labeler/keyspace_index_test.go (1)
58-118: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd unit test coverage for slot-collision rejection.
keyspaceRuleIndex.Addreturnsfalsewhen a different rule already occupies a slot (per its doc comment inkeyspace_index.go), but no test exercises this path — only the "non-canonical shape" rejection is covered here. A small case (e.g., two distinct canonical rules both computed for the same numeric id) would lock in the two-phase atomicity guarantee.🤖 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 `@pkg/schedule/labeler/keyspace_index_test.go` around lines 58 - 118, Extend TestKeyspaceRuleIndex with two distinct canonical rules targeting the same numeric keyspace ID, then assert the first Add succeeds, the second Add returns false, and the original rule remains indexed. Verify the rejected Add does not partially modify the index by checking Contains/GetRule and relevant split-key behavior.
🤖 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.
Inline comments:
In `@docs/keyspace-region-label-index.md`:
- Around line 33-38: Update the benchmark command sequence in the documentation
to run after make failpoint-enable and establish a cleanup trap that invokes
make failpoint-disable, ensuring cleanup occurs on exit. Keep both existing
benchmark commands unchanged and bracket the full sequence with the required
failpoint lifecycle.
---
Nitpick comments:
In `@pkg/schedule/labeler/keyspace_index_test.go`:
- Around line 58-118: Extend TestKeyspaceRuleIndex with two distinct canonical
rules targeting the same numeric keyspace ID, then assert the first Add
succeeds, the second Add returns false, and the original rule remains indexed.
Verify the rejected Add does not partially modify the index by checking
Contains/GetRule and relevant split-key behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 1898be2b-f010-461a-ac8b-39b5832f688d
📒 Files selected for processing (7)
docs/keyspace-region-label-index.mdpkg/schedule/labeler/keyspace_index.gopkg/schedule/labeler/keyspace_index_test.gopkg/schedule/labeler/labeler.gopkg/schedule/labeler/plan.gopkg/schedule/rangelist/range_list.gopkg/schedule/rangelist/range_list_test.go
Signed-off-by: Ryan Leung <rleungx@gmail.com>
Signed-off-by: Ryan Leung <rleungx@gmail.com>
|
/retest |
| if chunk.count == 0 { | ||
| s.chunks[chunkID] = nil | ||
| for len(s.chunks) > 0 && s.chunks[len(s.chunks)-1] == nil { | ||
| s.chunks = s.chunks[:len(s.chunks)-1] |
There was a problem hiding this comment.
P2: canonical-rule replacement still regresses for single-slot and high-ID chunks.
When the current rule is the only entry in its chunk, clear drops the chunk and the following set allocates a new roughly 8 KiB keyspaceRuleChunk. For a high ID, this loop also trims every trailing nil directory entry, then set grows the directory again. This is reachable rather than theoretical: NextGen uses SystemKeyspaceID = MaxValidKeyspaceID - 1.
I verified this against the current PR head with a temporary targeted test and benchmark:
- dense chunk:
99 ns/op, 0 B/op, 0 allocs/op - single low ID:
4552 ns/op, 9473 B/op, 1 alloc/op - single max ID:
14113 ns/op, 9472 B/op, 1 alloc/op
This contradicts the claimed allocation-free lock-held update path and leaves a sparse high-ID regression uncovered. Please avoid destroying/reallocating the sole chunk during replacement and avoid trimming then regrowing the high-ID directory; add a single-slot/high-ID benchmark or test.
There was a problem hiding this comment.
Fixed in a50a693. Canonical replacements now update owned slots in place, preserving the existing chunk and directory for both single-slot and max-ID rules. Added a zero-allocation reuse test for low and max IDs. Verification: full labeler/rangelist package tests passed; single-rule-update/100000 is 79.40 ns/op, 0 B/op, 0 allocs/op.
Signed-off-by: Ryan Leung <rleungx@gmail.com>
|
@rleungx: 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. |
2 participants
What problem does this PR solve?
Issue Number: Close #11030
Adding or deleting one canonical keyspace region-label rule rebuilt and sorted
the complete generic range index under the labeler lock. The cost and transient
memory therefore grew with every existing keyspace and was repeated by the
Scheduling Service watcher.
This is the upstream counterpart of tidbcloud/pd-cse#560.
What is changed and how does it work?
pkg/keyspace; all other rules retain the generic range-list behavior.
derive their split boundaries on demand.
rules, including Plan and watcher paths.
RawKV-disabled, RawKV-enabled, generic-rule, and priority semantics.
startup, lock-held update, and lookup benchmarks.
Check List
Tests
Self-checks:
lock-held update 156 ns / 163 ns and lookup 100 ns / 104 ns, all with 0 B
and 0 allocations per operation
Code changes
Side effects
user rules continue to use the existing range list.
Related changes
Release note
Summary by CodeRabbit
New Features
Bug Fixes
Performance
Tests