resourcecontrol: price NextGen remote reads separately#2022
resourcecontrol: price NextGen remote reads separately#2022YuhaoZhang00 wants to merge 1 commit into
Conversation
Signed-off-by: Yuhao Zhang <yhzhang00@outlook.com>
ti-chi-bot
Bot
added
the
dco-signoff: yes
|
[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
contribution
|
Hi @YuhaoZhang00. Thanks for your PR. I'm waiting for a tikv member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the 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
added
the
size/M
📝 WalkthroughWalkthroughNextGen coprocessor requests now advertise remote-read pricing, and response accounting derives a separate remote read-byte value from scan details while preserving legacy behavior. ChangesRemote read pricing
Estimated code review effort: 2 (Simple) | ~10 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant AttachContext
participant TiKV
participant MakeResponseInfo
Client->>AttachContext: attach NextGen coprocessor context
AttachContext->>TiKV: send ClientPricesRemoteReads flag
TiKV-->>MakeResponseInfo: return scan details
MakeResponseInfo-->>Client: expose read and remote read bytes
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
internal/resourcecontrol/resource_control_test.go (1)
110-124: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExplicitly test both
config.NextGenstates.By branching on the global
config.NextGenvalue, this test execution will only verify one of the behavior paths while silently skipping the other. Ifconfig.NextGenis a mutable variable, consider explicitly toggling it so that both legacy and NextGen behaviors are asserted in every test run.♻️ Proposed refactor to test both states
- info := MakeResponseInfo(resp) - if config.NextGen { - assert.Equal(t, uint64(100), info.ReadBytes()) - assert.Equal(t, uint64(40), info.RemoteReadBytes()) - } else { - assert.Equal(t, uint64(80), info.ReadBytes()) - assert.Zero(t, info.RemoteReadBytes()) - } + originalNextGen := config.NextGen + defer func() { config.NextGen = originalNextGen }() + + // Test NextGen behavior + config.NextGen = true + infoNext := MakeResponseInfo(resp) + assert.Equal(t, uint64(100), infoNext.ReadBytes()) + assert.Equal(t, uint64(40), infoNext.RemoteReadBytes()) + + // Test legacy behavior + config.NextGen = false + infoLegacy := MakeResponseInfo(resp) + assert.Equal(t, uint64(80), infoLegacy.ReadBytes()) + assert.Zero(t, infoLegacy.RemoteReadBytes())🤖 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 `@internal/resourcecontrol/resource_control_test.go` around lines 110 - 124, Update the test around MakeResponseInfo to explicitly run assertions for both config.NextGen values instead of branching on the current global state. Set config.NextGen to each state in turn, verify the corresponding ReadBytes and RemoteReadBytes results, and restore the original value afterward if needed.
🤖 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.
Nitpick comments:
In `@internal/resourcecontrol/resource_control_test.go`:
- Around line 110-124: Update the test around MakeResponseInfo to explicitly run
assertions for both config.NextGen values instead of branching on the current
global state. Set config.NextGen to each state in turn, verify the corresponding
ReadBytes and RemoteReadBytes results, and restore the original value afterward
if needed.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 8b75c4d7-12e6-471f-a7df-20fb818a84c8
⛔ Files ignored due to path filters (2)
go.sumis excluded by!**/*.sumintegration_tests/go.sumis excluded by!**/*.sum
📒 Files selected for processing (6)
go.modintegration_tests/go.modinternal/resourcecontrol/resource_control.gointernal/resourcecontrol/resource_control_test.gotikvrpc/tikvrpc.gotikvrpc/tikvrpc_test.go
1 participant
What problem does this PR solve?
Issue Number: ref tikv/pd#11019
Related protocol: pingcap/kvproto#1500
Related RU-v1 pricing model: tikv/pd#11018
NextGen remote coprocessor responses need to keep scan statistics factual while allowing the RU-v1 controller to price the remote byte subset separately. Legacy clients must retain the existing pre-priced compatibility behavior.
What is changed?
client_prices_remote_readsonly for NextGen coprocessor requests. Non-NextGen and non-coprocessor requests keep the capability disabled.ReadBytes()as the factual maximum of total/processed scan bytes and expose the factual remote maximum throughRemoteReadBytes()for PD's optional remote-byte provider interface.There is no RU-v2 pricing or accounting change, and client-go does not multiply facts by a discount.
Tests
go test --tags=intest ./...go test --tags=intest ./internal/resourcecontrol ./tikvrpcgo test --tags='intest nextgen' ./internal/resourcecontrol ./tikvrpcgo test -race --tags=intest ./internal/resourcecontrol ./tikvrpcgo test -race --tags='intest nextgen' ./internal/resourcecontrol ./tikvrpcgo generate ./...The repository-wide NextGen-tag run has one unrelated baseline failure in
internal/locate(TestRegionRequestToThreeStores/TestDoNotTryUnreachableLeader); the same assertion reproduces on a clean detachedupstream/master. All changed NextGen packages pass.Summary by CodeRabbit
New Features
Bug Fixes
Tests