resource_control: add paging bytes pre-charge in RU calculation#10548
resource_control: add paging bytes pre-charge in RU calculation#10548JmPotato wants to merge 2 commits intotikv:masterfrom
Conversation
Add PagingSizeBytes() to RequestInfo interface. When a read request carries a byte budget (from RC paging), BeforeKVRequest pre-charges the estimated read RU in Phase 1, and AfterKVRequest subtracts it in Phase 2 to maintain correct total cost. This makes concurrent workers throttle at Phase 1 instead of all hitting Phase 2 simultaneously. Signed-off-by: JmPotato <github@ipotato.me>
Test that BeforeKVRequest pre-charges pagingSizeBytes * ReadBytesCost, AfterKVRequest subtracts it, and the net total equals baseCost + actualCost. Signed-off-by: JmPotato <github@ipotato.me>
ti-chi-bot
bot
added
do-not-merge/needs-linked-issue
release-note-none
|
[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 |
📝 WalkthroughWalkthroughThis pull request implements a paging size bytes pre-charge mechanism for KV RRU (Resource Request Unit) accounting. The Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested labels
Suggested reviewers
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)
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
|
[FORMAT CHECKER NOTIFICATION] Notice: To remove the 📖 For more info, you can check the "Linking issues" section in the CONTRIBUTING.md. |
![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)
client/resource_group/controller/group_controller_test.go (1)
226-226: Use thedefaultAvgBatchProportionconstant instead of hardcoding0.7.The test is in the same package as
config.gowhere this constant is defined, so it can be referenced directly. This keeps the test aligned with the actual default value and avoids duplication.♻️ Suggested fix
- baseCost := float64(cfg.ReadBaseCost) + float64(cfg.ReadPerBatchBaseCost)*0.7 + baseCost := float64(cfg.ReadBaseCost) + float64(cfg.ReadPerBatchBaseCost)*defaultAvgBatchProportion🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@client/resource_group/controller/group_controller_test.go` at line 226, Replace the hardcoded 0.7 used when computing baseCost with the package constant defaultAvgBatchProportion so the test uses the canonical default; specifically, update the expression computing baseCost (which uses cfg.ReadBaseCost and cfg.ReadPerBatchBaseCost) to multiply cfg.ReadPerBatchBaseCost by defaultAvgBatchProportion instead of 0.7.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@client/resource_group/controller/model.go`:
- Around line 55-57: The new PagingSizeBytes() uint64 method was added to the
RequestInfo interface which will break external implementations; update any
external/custom types that implement RequestInfo (and ensure TestRequestInfo
remains correct) to add a PagingSizeBytes() method returning the intended byte
budget (0 for no pre-charge) or provide a default adapter that satisfies
RequestInfo; locate implementations by searching for RequestInfo,
TestRequestInfo, and any concrete types used where RequestInfo is expected and
add the PagingSizeBytes method with the correct semantics before merging.
---
Nitpick comments:
In `@client/resource_group/controller/group_controller_test.go`:
- Line 226: Replace the hardcoded 0.7 used when computing baseCost with the
package constant defaultAvgBatchProportion so the test uses the canonical
default; specifically, update the expression computing baseCost (which uses
cfg.ReadBaseCost and cfg.ReadPerBatchBaseCost) to multiply
cfg.ReadPerBatchBaseCost by defaultAvgBatchProportion instead of 0.7.
🪄 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: 2cdc3320-aa36-4f9e-b292-00f712694795
📒 Files selected for processing (3)
client/resource_group/controller/group_controller_test.goclient/resource_group/controller/model.goclient/resource_group/controller/testutil.go
| // PagingSizeBytes returns the byte budget per page for RC paging. | ||
| // 0 means no byte-based pre-charge estimation is applied. | ||
| PagingSizeBytes() uint64 |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for types that might implement the RequestInfo interface
# by looking for methods like IsWrite() bool which is part of the interface
rg -n --type=go 'func \([^)]+\) IsWrite\(\) bool' -A1 -B2Repository: tikv/pd
Length of output: 357
🏁 Script executed:
#!/bin/bash
# Find the RequestInfo interface definition
rg -n --type=go 'type RequestInfo interface' -A 20Repository: tikv/pd
Length of output: 1733
🏁 Script executed:
#!/bin/bash
# Search for all types that implement RequestInfo by looking for struct types with methods
# that match interface signatures (IsWrite, GetWrite, GetRead)
rg -n --type=go 'func \([^)]+\) (IsWrite|GetWrite|GetRead|PagingSizeBytes)\('Repository: tikv/pd
Length of output: 246
🏁 Script executed:
#!/bin/bash
# Check if TestRequestInfo has PagingSizeBytes method
cat -n client/resource_group/controller/testutil.go | head -100Repository: tikv/pd
Length of output: 3644
🏁 Script executed:
#!/bin/bash
# Search for all methods that are part of RequestInfo interface
# to identify all types implementing it
rg -n --type=go 'func \([^)]+\) (WriteBytes|ReplicaNumber|StoreID|RequestSize|AccessLocationType|PagingSizeBytes)\(\) ' | grep -v 'interface' | cut -d':' -f1 | sort -uRepository: tikv/pd
Length of output: 95
🏁 Script executed:
#!/bin/bash
# Look for any new struct types defined that might implement RequestInfo
rg -n --type=go 'type \w+ struct' | head -30Repository: tikv/pd
Length of output: 2146
🏁 Script executed:
#!/bin/bash
# Check if there are any comments or patterns suggesting external RequestInfo implementations
rg -n --type=go 'RequestInfo' -B2 -A2Repository: tikv/pd
Length of output: 50363
🏁 Script executed:
#!/bin/bash
# Look more carefully for any type that could implement RequestInfo
# by searching for functions with exact matching signatures
rg -n --type=go 'func \([^)]+\) (IsWrite|WriteBytes|ReplicaNumber|StoreID|RequestSize|AccessLocationType|PagingSizeBytes)\(\)' --no-heading | cut -d':' -f1 | sort | uniq -cRepository: tikv/pd
Length of output: 103
🏁 Script executed:
#!/bin/bash
# Check if there are any interfaces embedded or other implementations
rg -n --type=go 'RequestInfo' client/resource_group/controller/ | grep -v 'testutil\|model.go' | head -20Repository: tikv/pd
Length of output: 2392
Interface change is intentionally breaking for external implementations only; internal codebase is properly updated.
Adding PagingSizeBytes() to the RequestInfo interface breaks external code implementing this interface, but the internal test implementation (TestRequestInfo) has already been updated with this method. This appears to be an intentional breaking change based on PR objectives mentioning backward compatibility impacts.
If external consumers (TiDB, TiKV client code) have custom RequestInfo implementations, ensure they are updated to include the PagingSizeBytes() method before merging.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@client/resource_group/controller/model.go` around lines 55 - 57, The new
PagingSizeBytes() uint64 method was added to the RequestInfo interface which
will break external implementations; update any external/custom types that
implement RequestInfo (and ensure TestRequestInfo remains correct) to add a
PagingSizeBytes() method returning the intended byte budget (0 for no
pre-charge) or provide a default adapter that satisfies RequestInfo; locate
implementations by searching for RequestInfo, TestRequestInfo, and any concrete
types used where RequestInfo is expected and add the PagingSizeBytes method with
the correct semantics before merging.
|
@JmPotato: 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. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #10548 +/- ##
==========================================
+ Coverage 78.88% 78.96% +0.08%
==========================================
Files 530 532 +2
Lines 71548 71808 +260
==========================================
+ Hits 56439 56705 +266
+ Misses 11092 11074 -18
- Partials 4017 4029 +12
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
1 participant
What problem does this PR solve?
Issue Number: ref pingcap/kvproto#1448
Implement byte-budget RU pre-charge and settlement in the resource controller's KV calculator to support RC paging.
What is changed and how does it work?
Add a two-phase RU charging model for RC paging based on
PagingSizeBytes:Phase 1 (BeforeKVRequest - Pre-charge):
pagingSizeBytes > 0, pre-charge estimated read RU:RRU += ReadBytesCost × pagingSizeBytesPhase 2 (AfterKVRequest - Settlement):
RRU -= ReadBytesCost × pagingSizeBytesbaseCost + actualReadCost + cpuCost(no double-counting)Changes:
RequestInfointerface: addPagingSizeBytes() uint64methodKVCalculator.BeforeKVRequest: pre-charge byte-budget RRU when paging is enabledKVCalculator.AfterKVRequest: subtract pre-charged amount during settlementCheck List
Tests
Code changes
Side effects
Related changes
pingcap/docs/pingcap/docs-cn:pingcap/tiup:Release note
Summary by CodeRabbit
Tests
Refactor