client/resource_group: settle write consumption at response time - #10986
client/resource_group: settle write consumption at response time#10986YuhaoZhang00 wants to merge 5 commits into
Conversation
Signed-off-by: Yuhao Zhang <yhzhang00@outlook.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
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/L
|
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:
📝 WalkthroughWalkthroughWrite WRU and WriteBytes reporting now excludes reservations during the request phase and adds them during response processing. Unit and controller tests verify successful settlement, failed-response deltas, and delayed periodic reporting. ChangesWrite consumption accounting
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Suggested labels: 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 |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #10986 +/- ##
==========================================
- Coverage 79.21% 79.18% -0.04%
==========================================
Files 541 541
Lines 75677 75726 +49
==========================================
+ Hits 59949 59960 +11
- Misses 11491 11524 +33
- Partials 4237 4242 +5
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.
🧹 Nitpick comments (1)
client/resource_group/controller/model.go (1)
196-224: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDuplicated calculator-loop between reservation and refund helpers.
writeReservationConsumptionandwriteRefundConsumptionare structurally identical except for theKVCalculatormethod invoked. Consider extracting the common loop/type-assertion into a small helper parameterized by the calculator method to reduce duplication.♻️ Example refactor
+func applyWriteAdjustment(calculators []ResourceCalculator, req RequestInfo, fn func(*KVCalculator, *rmpb.Consumption, RequestInfo)) *rmpb.Consumption { + adjustment := &rmpb.Consumption{} + if !req.IsWrite() { + return adjustment + } + for _, calc := range calculators { + if kvCalc, ok := calc.(*KVCalculator); ok { + fn(kvCalc, adjustment, req) + } + } + return adjustment +} + -func writeReservationConsumption(calculators []ResourceCalculator, req RequestInfo) *rmpb.Consumption { - reservation := &rmpb.Consumption{} - if !req.IsWrite() { - return reservation - } - for _, calc := range calculators { - kvCalc, ok := calc.(*KVCalculator) - if !ok { - continue - } - kvCalc.calculateWriteCost(reservation, req) - } - return reservation -} - -func writeRefundConsumption(calculators []ResourceCalculator, req RequestInfo) *rmpb.Consumption { - refund := &rmpb.Consumption{} - if !req.IsWrite() { - return refund - } - for _, calc := range calculators { - kvCalc, ok := calc.(*KVCalculator) - if !ok { - continue - } - kvCalc.payBackWriteCost(refund, req) - } - return refund -} +func writeReservationConsumption(calculators []ResourceCalculator, req RequestInfo) *rmpb.Consumption { + return applyWriteAdjustment(calculators, req, (*KVCalculator).calculateWriteCost) +} + +func writeRefundConsumption(calculators []ResourceCalculator, req RequestInfo) *rmpb.Consumption { + return applyWriteAdjustment(calculators, req, (*KVCalculator).payBackWriteCost) +}🤖 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 `@client/resource_group/controller/model.go` around lines 196 - 224, Both writeReservationConsumption and writeRefundConsumption duplicate the same ResourceCalculator loop and KVCalculator type assertion, differing only in the method invoked. Extract the shared iteration and type-checking logic in model.go into a small helper around KVCalculator, then have writeReservationConsumption call the helper with calculateWriteCost and writeRefundConsumption call it with payBackWriteCost to remove duplication while preserving 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.
Nitpick comments:
In `@client/resource_group/controller/model.go`:
- Around line 196-224: Both writeReservationConsumption and
writeRefundConsumption duplicate the same ResourceCalculator loop and
KVCalculator type assertion, differing only in the method invoked. Extract the
shared iteration and type-checking logic in model.go into a small helper around
KVCalculator, then have writeReservationConsumption call the helper with
calculateWriteCost and writeRefundConsumption call it with payBackWriteCost to
remove duplication while preserving behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a7b296f1-4dfd-49df-8bd0-9233fbce3586
📒 Files selected for processing (3)
client/resource_group/controller/group_controller.goclient/resource_group/controller/group_controller_test.goclient/resource_group/controller/model.go
Signed-off-by: Yuhao Zhang <yhzhang00@outlook.com>
ti-chi-bot
Bot
added
size/XL
Signed-off-by: Yuhao Zhang <yhzhang00@outlook.com>
|
Conflict-resolution note: Expected conflicts after rebasing this PR on top of #10611:
Resolve them by keeping both semantics:
After resolving, rerun the targeted |
Merge the current upstream master and keep request-time write admission reservation separate from reported consumption. Restore the reservation in the response-side settlement delta without branching on response success. Signed-off-by: Yuhao Zhang <yhzhang00@outlook.com>
ti-chi-bot
Bot
added
size/L
YuhaoZhang00
changed the title
Signed-off-by: Yuhao Zhang <yhzhang00@outlook.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@client/resource_group/controller/group_controller.go`:
- Around line 689-690: The response settlement logic around reportedDelta in
both client/resource_group/controller/group_controller.go lines 689-690 and
727-728 must refund every negative delta, including failed-write paybacks,
rather than restricting RefundTokens to paging reads. Update both response paths
consistently while preserving the existing paging overestimate refund 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 Plus
Run ID: 27e0b2ac-4fd1-45db-a1c9-30ccc275e089
📒 Files selected for processing (3)
client/resource_group/controller/group_controller.goclient/resource_group/controller/model.goclient/resource_group/controller/model_test.go
| var reported rmpb.Consumption | ||
| reportedDelta := reportedResponseConsumption(gc.calculators, req, delta, &reported) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Refund every negative settlement delta, not only paging deltas. Failed writes reserve tokens in onRequestWaitImpl, then AfterKVRequest applies negative write payback. Both response paths currently skip RefundTokens unless the request is a paging read.
client/resource_group/controller/group_controller.go#L689-L690: refund negative failed-write deltas in addition to paging overestimates.client/resource_group/controller/group_controller.go#L727-L728: apply the same refund behavior in the response-wait path.
Proposed fix
- } else if v < 0 && isPagingRead && bytesForEst > 0 {
+ } else if v < 0 {
counter.limiter.RefundTokens(time.Now(), -v)
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| var reported rmpb.Consumption | |
| reportedDelta := reportedResponseConsumption(gc.calculators, req, delta, &reported) | |
| } else if v < 0 { | |
| counter.limiter.RefundTokens(time.Now(), -v) | |
| } |
📍 Affects 1 file
client/resource_group/controller/group_controller.go#L689-L690(this comment)client/resource_group/controller/group_controller.go#L727-L728
🤖 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 `@client/resource_group/controller/group_controller.go` around lines 689 - 690,
The response settlement logic around reportedDelta in both
client/resource_group/controller/group_controller.go lines 689-690 and 727-728
must refund every negative delta, including failed-write paybacks, rather than
restricting RefundTokens to paging reads. Update both response paths
consistently while preserving the existing paging overestimate refund behavior.
1 participant
What problem does this PR solve?
Issue Number: Close #10985
OnRequestWaitmust reserve the full write cost before sending a KV RPC so the local limiter can enforce admission control. The same reservation was also entering cumulative reported consumption immediately, which coupled token reservation with settled consumption. Successful write WRU could be reported before the response, and any existing failed-write payback that crossed a periodic reporting boundary could not correct the already reported positive delta because consumption reporting is monotonic.What is changed and how does it work?
This PR keeps request-time write token reservation unchanged but removes the write reservation from request-side reported consumption. The response-side reporting helper restores the reservation unconditionally, so the reservation and the existing response delta settle together in one reporting interval. Successful writes report their full reservation at response settlement; when the controller receives a failed response, its existing payback is combined with the reservation before reporting instead of depending on a later negative correction.
The reporting split no longer depends on
ResponseInfo.Succeed()to decide whether to restore the reservation. Determining success inclient-goand handling requests with no response are explicitly outside this PR scope. Because the same settled-consumption counter also feeds the controller average RU observation and reporting threshold, write consumption becomes visible to those paths at response settlement as well; request-time admission remains protected by the unchanged local limiter reservation.Check List
Tests
Code changes
Side effects
client-gosuccess detection and no-response behavior are unchanged.Related changes
Release note
Summary by CodeRabbit