client/resource_group: delay reporting write reservation#10986
client/resource_group: delay reporting write reservation#10986YuhaoZhang00 wants to merge 3 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
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughThis PR updates failed-write WRU accounting so controller reporting uses reported consumption views, and local limiter state can refund write reservations when a write response fails. ChangesFailed write WRU accounting
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant groupCostController
participant model as model.go helpers
participant Limiter
participant gc_mu as gc.mu.consumption
Client->>groupCostController: onRequestWaitImpl
groupCostController->>model: reportedRequestConsumption(tokenDelta, req)
model-->>groupCostController: reportedDelta
groupCostController->>gc_mu: record reportedDelta
groupCostController->>Limiter: reserve write tokens
Client->>groupCostController: onResponseImpl / onResponseWaitImpl
groupCostController->>model: reportedResponseConsumption(tokenDelta, req, resp)
model-->>groupCostController: reportedDelta
groupCostController->>gc_mu: record reportedDelta
alt write failed
groupCostController->>Limiter: RefundTokens(now, refundAmount)
else write succeeded
groupCostController->>Limiter: remove or acquire tokens
end
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 |
1 participant
What problem does this PR solve?
Issue Number: Close #10985
Failed write requests can leave two related accounting artifacts behind:
What is changed and how does it work?
This PR keeps using the full write reservation for token acquisition, but delays reporting the write reservation as actual consumption until the response confirms success. For failed writes, the response-side payback is removed from the reported actual consumption path so it does not become negative reported usage.
This PR also refunds the failed-write response-side payback delta to the local limiter. The limiter refund primitive is covered directly, and read requests skip the write-only helper path without allocating an empty consumption object.
Check List
Tests
Code changes
Side effects
Related changes
Release note
Summary by CodeRabbit
New Features
Bug Fixes
Tests