client/resource_group: add RC paging pre-charge with PredictedReadBytes hint#10611
Conversation
Introduce an optional predictedReadBytesProvider interface on RequestInfo. When a caller (e.g. TiDB maintaining a per-logical-scan EMA across paging RPCs) supplies a non-zero PredictedReadBytes, BeforeKVRequest pre-charges ReadBytesCost * predictedReadBytes RRU so that concurrent workers are throttled at BeforeKVRequest rather than all hitting AfterKVRequest settlement at once. AfterKVRequest then subtracts the same basis so the net (pre-charge + settlement) equals baseCost + actualCost. Without a hint the request is not pre-charged and is billed in AfterKVRequest by actual read bytes only - this keeps the RU-billing pre-charge decoupled from the protocol-level paging byte cap. The hint is added as an optional interface (not a method on RequestInfo) so existing RequestInfo implementations compile unchanged; they simply skip pre-charge. Signed-off-by: Yuhao Zhang <yhzhang00@outlook.com>
…ment When a request carries a PredictedReadBytes hint, BeforeKVRequest consumes tokens up-front as a pre-charge. If the actual read bytes come back smaller than the estimate, the delta represents tokens that were reserved but never consumed. Previously AfterKVRequest computed a negative delta but called RemoveTokens unconditionally, which further debited the limiter instead of giving tokens back. This commit adds Limiter.RefundTokens as the inverse of RemoveTokens and wires the response-side paths (onResponseImpl, onResponseWaitImpl) to call it whenever the settlement delta is negative, so over-estimated pre-charges are released back to the group's token bucket. Signed-off-by: Yuhao Zhang <yhzhang00@outlook.com>
Add per-resource-group Prometheus metrics so operators can observe how the
paging pre-charge path behaves in production and judge EMA prediction
accuracy:
- paging_precharge_total / paging_precharge_bytes_total: count and byte
volume of RPCs that arrived with a PredictedReadBytes hint > 0 and
were pre-charged at BeforeKVRequest.
- paging_actual_bytes_total: actual read bytes reported by pre-charged
RPCs, to compute an over/under-charge ratio against the pre-charge
volume.
- paging_prediction_residual_bytes: histogram of (actual - predicted)
bytes for pre-charged RPCs; shows EMA prediction accuracy directly.
- paging_nonprecharge_total / paging_nonprecharge_actual_bytes_total:
count and byte volume of read RPCs that implemented the predicted
hint interface but reported 0 (e.g. EMA cold-start) and therefore
ran without pre-charge. Paired with paging_precharge_total this
yields the cold/ready RPC split from the PD client side.
Labels are cached per resource group in groupMetricsCollection to keep
the hot path out of WithLabelValues. Only RequestInfo implementations
that opt into the PredictedReadBytes interface contribute to these
series; existing callers are unaffected.
Signed-off-by: Yuhao Zhang <yhzhang00@outlook.com>
ti-chi-bot
Bot
added
do-not-merge/needs-linked-issue
release-note-none
|
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/XL
📝 WalkthroughWalkthroughAdds an opt-in RC paging pre-charge mechanism: a ChangesClient-side RC paging pre-charge feature
Server-side metering nil-safety and RU clamping
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant GroupController
participant Limiter
participant Metrics
Client->>GroupController: onRequestWaitImpl(info)
GroupController->>GroupController: pagingReadEstimate(info)
GroupController->>Limiter: acquireTokens(precharge RU)
GroupController->>Metrics: observePagingRequest(estimate)
Client->>GroupController: onResponseWaitImpl/onResponseImpl(req, resp)
GroupController->>GroupController: settle delta via BeforeKVRequest/AfterKVRequest
alt settlement negative (over-estimate)
GroupController->>Limiter: RefundTokens(amount)
else settlement positive
GroupController->>Limiter: acquireTokens/RemoveTokens
end
GroupController->>Metrics: observePagingResponse(actual, residual)
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Warning Tools execution failed with the following error: Failed to run tools: 13 INTERNAL: Received RST_STREAM with code 2 (Internal server error) 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.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
client/resource_group/controller/group_controller.go (1)
602-626:⚠️ Potential issue | 🟡 MinorPre-charge counter increments even when the request is then throttled.
observePagingPrechargeruns beforeacquireTokens. IfacquireTokensfails withErrClientResourceGroupThrottled,mu.consumptionis rolled back viasub(...), butPagingPrechargeCounter/PagingPrechargeBytesCounterare already incremented, andonResponseImpl/onResponseWaitImplwill never run for this request to balance it with aPagingActualBytesCounterentry. Dashboards comparing actual-vs-precharge bytes (the documented use of these metrics) will show a chronic deficit proportional to throttling rate.Consider moving the observation to after a successful acquire (or skipping it on the throttled-rollback path), or alternatively add a separate
PagingPrechargeThrottledCounterso the denominator stays honest.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@client/resource_group/controller/group_controller.go` around lines 602 - 626, The pre-charge metrics are recorded by observePagingPrecharge before calling acquireTokens, causing unbalanced precharge counts when acquireTokens fails and consumption is rolled back (sub on gc.mu.consumption); update the logic in group_controller.go so that observePagingPrecharge is only called after a successful acquireTokens (i.e., move the observePagingPrecharge call to after acquireTokens returns without error), or alternatively decrement or record a separate PagingPrechargeThrottledCounter in the error branch that handles errs.ErrClientResourceGroupThrottled (the branch that calls sub and returns the error) to keep PagingPrechargeCounter/PagingPrechargeBytesCounter consistent with actual completed requests.
🤖 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/group_controller.go`:
- Around line 602-604: The metric observations (calls to
gc.metrics.observePagingPrecharge / observePagingPrechargeBytes /
observePagingActualBytes / observePagingPredictionResidual) should only run for
read requests to match the pre-charge behavior in
BeforeKVRequest/AfterKVRequest; update the branches that call
estimatedReadBytes(info) (notably in onResponseImpl and onResponseWaitImpl and
the other occurrences mentioned) to first check that the request is a read
(e.g., !req.IsWrite()) before observing any paging metrics, so pre-charge and
metric recording are gated symmetrically with the RU accounting in model.go.
---
Outside diff comments:
In `@client/resource_group/controller/group_controller.go`:
- Around line 602-626: The pre-charge metrics are recorded by
observePagingPrecharge before calling acquireTokens, causing unbalanced
precharge counts when acquireTokens fails and consumption is rolled back (sub on
gc.mu.consumption); update the logic in group_controller.go so that
observePagingPrecharge is only called after a successful acquireTokens (i.e.,
move the observePagingPrecharge call to after acquireTokens returns without
error), or alternatively decrement or record a separate
PagingPrechargeThrottledCounter in the error branch that handles
errs.ErrClientResourceGroupThrottled (the branch that calls sub and returns the
error) to keep PagingPrechargeCounter/PagingPrechargeBytesCounter consistent
with actual completed requests.
🪄 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: f857a943-c822-4ef7-939c-79712b2a1f70
📒 Files selected for processing (7)
client/resource_group/controller/group_controller.goclient/resource_group/controller/group_controller_test.goclient/resource_group/controller/limiter.goclient/resource_group/controller/limiter_test.goclient/resource_group/controller/metrics/metrics.goclient/resource_group/controller/model.goclient/resource_group/controller/testutil.go
Temporarily replace github.com/tikv/client-go/v2 and github.com/tikv/pd/client with the corresponding forks carrying the PredictedReadBytes hint and controller-side pre-charge logic so CI can build this PR before the two upstream PRs land: - github.com/YuhaoZhang00/client-go/v2 (tikv/client-go#1947) - github.com/YuhaoZhang00/pd/client (tikv/pd#10611) Will be reverted and replaced with a normal require bump once both PRs merge and tag new releases. Signed-off-by: Yuhao Zhang <yhzhang00@outlook.com>
Temporarily replace github.com/tikv/client-go/v2 and github.com/tikv/pd/client with the corresponding forks carrying the PredictedReadBytes hint and controller-side pre-charge logic so CI can build this PR before the two upstream PRs land: - github.com/YuhaoZhang00/client-go/v2 (tikv/client-go#1947) - github.com/YuhaoZhang00/pd/client (tikv/pd#10611) Will be reverted and replaced with a normal require bump once both PRs merge and tag new releases. Signed-off-by: Yuhao Zhang <yhzhang00@outlook.com>
ti-chi-bot
Bot
added
do-not-merge/needs-triage-completed
and removed
do-not-merge/needs-linked-issue
labels
Signed-off-by: Yuhao Zhang <yhzhang00@outlook.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
client/resource_group/controller/limiter.go (1)
328-340: LGTM on the refund primitive; consider symmetry note.Implementation mirrors
RemoveTokenswith the opposite sign and skipsmaybeNotify(refund can't make us low), which is the right call. Sincetokenscan temporarily exceedburst, the cap is applied implicitly on the nextgetTokens()call — that's fine but worth a one-line comment so future readers don't mistake it for a leak of the burst cap.📝 Optional doc tweak
// RefundTokens adds tokens back to the limiter. // -// No burst cap is applied here +// No burst cap is applied here; any overshoot is clamped to burst on the +// next getTokens() call. Does not call maybeNotify since adding tokens +// cannot put the limiter into the low-token state. func (lim *Limiter) RefundTokens(now time.Time, amount float64) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@client/resource_group/controller/limiter.go` around lines 328 - 340, The RefundTokens implementation mirrors RemoveTokens but omits maybeNotify and allows lim.tokens to temporarily exceed lim.burst because the cap is enforced later in getTokens; add a single-line comment inside RefundTokens (near the lim.tokens = tokens + amount line) stating that exceeding burst is intentional and will be capped by getTokens on the next access, and note that maybeNotify is intentionally not called for refunds to clarify intent for future readers (reference symbols: RefundTokens, RemoveTokens, maybeNotify, getTokens, burst).
🤖 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/group_controller.go`:
- Around line 689-710: The change skips calling acquireTokens and recording
successfulRequestDuration when getRUValueFromConsumption(delta) returns exactly
0; to preserve prior metrics behavior, add a v == 0 branch after the existing
if/else-if so that when v == 0 you call
gc.metrics.successfulRequestDuration.Observe(0) (and leave waitDuration
unchanged), referencing the existing symbols getRUValueFromConsumption,
acquireTokens, and gc.metrics.successfulRequestDuration.Observe to locate where
to add the single-line observation.
---
Nitpick comments:
In `@client/resource_group/controller/limiter.go`:
- Around line 328-340: The RefundTokens implementation mirrors RemoveTokens but
omits maybeNotify and allows lim.tokens to temporarily exceed lim.burst because
the cap is enforced later in getTokens; add a single-line comment inside
RefundTokens (near the lim.tokens = tokens + amount line) stating that exceeding
burst is intentional and will be capped by getTokens on the next access, and
note that maybeNotify is intentionally not called for refunds to clarify intent
for future readers (reference symbols: RefundTokens, RemoveTokens, maybeNotify,
getTokens, burst).
🪄 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: 4f54c1ea-7b65-4175-ba34-2b2aaf771bc9
📒 Files selected for processing (4)
client/resource_group/controller/group_controller.goclient/resource_group/controller/limiter.goclient/resource_group/controller/metrics/metrics.goclient/resource_group/controller/model.go
🚧 Files skipped from review as they are similar to previous changes (1)
- client/resource_group/controller/metrics/metrics.go
| if bytesForEst := estimatedReadBytes(req); bytesForEst > 0 { | ||
| gc.metrics.observePagingActual(bytesForEst, resp.ReadBytes()) | ||
| } else if !req.IsWrite() { | ||
| if _, ok := req.(predictedReadBytesProvider); ok { |
There was a problem hiding this comment.
Looking at the code, all requests implement this interface, meaning they will all enter this branch under any path.
Given that, what is the purpose of this non-precharge metric?
The previous +/-4MB range clipped large first-page responses on cold copIterators (predicted=0 -> residual == actual, commonly several MB) and workload shifts where EMA sits above the current actual. Extending both ends by two factor-4 steps keeps the same near-zero resolution while making P95/P99 readable up to the TiKV paging cap. Signed-off-by: Yuhao Zhang <yhzhang00@outlook.com>
EMA is a TiDB-side implementation detail; PD's metric Help text should describe what the metric observes in terms of the hint contract. paging_nonprecharge_* also fires when hint is absent entirely (not just when the predictor produced 0), so reword to say so. Signed-off-by: Yuhao Zhang <yhzhang00@outlook.com>
ti-chi-bot
Bot
added
size/XXL
YuhaoZhang00
force-pushed
the
feat/rc-paging-precharge
branch
from
a8777a9 to
1764549
Compare
ti-chi-bot
Bot
added
size/XL
There was a problem hiding this comment.
♻️ Duplicate comments (1)
client/resource_group/controller/group_controller.go (1)
601-627:⚠️ Potential issue | 🟡 MinorAlign paging metrics with successful read pre-charges.
Line 601 records pre-charge metrics before token acquisition can fail, so throttled/canceled requests inflate
paging_precharge_*without a matching settlement sample. Also, settlement/precharge observations key offestimatedReadBytes > 0without the read guard used byBeforeKVRequest, so a write request implementing the optional predictor could corrupt paging metrics.🛡️ Proposed metric-gating adjustment
for _, calc := range gc.calculators { calc.BeforeKVRequest(delta, info) } - if bytesForEst := estimatedReadBytes(info); bytesForEst > 0 { - gc.metrics.observePagingPrecharge(bytesForEst, getRUValueFromConsumption(delta)) - } + bytesForEst := estimatedReadBytes(info) + prechargeRU := getRUValueFromConsumption(delta) gc.mu.Lock() add(gc.mu.consumption, delta) gc.mu.Unlock() @@ gc.metrics.successfulRequestDuration.Observe(d.Seconds()) waitDuration += d } + if bytesForEst > 0 && !info.IsWrite() { + gc.metrics.observePagingPrecharge(bytesForEst, prechargeRU) + }- if bytesForEst := estimatedReadBytes(req); bytesForEst > 0 { - gc.metrics.observePagingActual(bytesForEst, resp.ReadBytes(), - getRUValueFromConsumption(count), getRUValueFromConsumption(delta)) - } else if !req.IsWrite() { - if _, ok := req.(predictedReadBytesProvider); ok { - gc.metrics.observePagingNonprecharge(resp.ReadBytes()) + if !req.IsWrite() { + if bytesForEst := estimatedReadBytes(req); bytesForEst > 0 { + gc.metrics.observePagingActual(bytesForEst, resp.ReadBytes(), + getRUValueFromConsumption(count), getRUValueFromConsumption(delta)) + } else if _, ok := req.(predictedReadBytesProvider); ok { + gc.metrics.observePagingNonprecharge(resp.ReadBytes()) } }Apply the same response-side shape in
onResponseWaitImpl.Also applies to: 661-667, 702-708
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@client/resource_group/controller/group_controller.go` around lines 601 - 627, The paging pre-charge metrics are recorded too early and without the same read-only guard used elsewhere: move the gc.metrics.observePagingPrecharge(...) call so it runs only after acquireTokens(...) succeeds (i.e., after the error check where d and err are handled) and gate it with the same read-only check used by BeforeKVRequest (use estimatedReadBytes(info) > 0 combined with the request read-predictor guard) so write requests or failed token acquisitions do not increment paging_precharge_*; make the identical adjustment in onResponseWaitImpl for the other occurrences referenced (around the 661-667 and 702-708 regions) to keep precharge and settlement metrics symmetric.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@client/resource_group/controller/group_controller.go`:
- Around line 601-627: The paging pre-charge metrics are recorded too early and
without the same read-only guard used elsewhere: move the
gc.metrics.observePagingPrecharge(...) call so it runs only after
acquireTokens(...) succeeds (i.e., after the error check where d and err are
handled) and gate it with the same read-only check used by BeforeKVRequest (use
estimatedReadBytes(info) > 0 combined with the request read-predictor guard) so
write requests or failed token acquisitions do not increment paging_precharge_*;
make the identical adjustment in onResponseWaitImpl for the other occurrences
referenced (around the 661-667 and 702-708 regions) to keep precharge and
settlement metrics symmetric.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 08e17f51-e08f-428b-bb0d-1e6f02ab316b
📥 Commits
Reviewing files that changed from the base of the PR and between 1764549 and a8777a9e370b2d5bec550444d2c9501b5893b182.
📒 Files selected for processing (2)
client/resource_group/controller/group_controller.goclient/resource_group/controller/metrics/metrics.go
🚧 Files skipped from review as they are similar to previous changes (1)
- client/resource_group/controller/metrics/metrics.go
|
/retest |
Signed-off-by: Yuhao Zhang <yhzhang00@outlook.com>
JmPotato
left a comment
JmPotato
left a comment
There was a problem hiding this comment.
Did another pass after the SettlementSinceLastRequest removal — the wire-protocol concern is fully resolved, and I verified the reported consumption for pre-charged reads now matches master exactly (request = base cost, response = actual + CPU; the pre-charge never reaches billing/stats). Remaining items, in priority order:
Blocker — go.sum is not tidy. The go.mod files of the root, tests/integrations, and tools modules are untouched, yet each of their go.sum files still carries ~800 added lines of stale checksums left over from the fork-replace era. A module whose go.mod didn't change should have zero go.sum diff, so make tidy will prune all of them and the CI clean-diff check is likely to fail as-is. Please restore the three go.sum files and re-run go mod tidy in each module. (Promoting github.com/prometheus/client_model to a direct dependency in client/go.mod is fine — the new tests use dto.Metric directly.)
PR description drift. The description still says PredictedReadBytes is "a required method on the interface" (it is an optional type assertion now, together with the IsCop() gate which isn't mentioned at all), still scopes the change to client/resource_group/controller (server-side files are touched), and the test list references TestPagingPreChargeZeroDelta, which no longer exists under that name. Please refresh it to match the current implementation.
The inline comments below cover settlement-era leftovers that can be reverted now, plus a few behavior changes that deserve either a split or an explicit call-out.
| return ticker | ||
| } | ||
|
|
||
| func resourceGroupPushCollectors() []prometheus.Collector { |
There was a problem hiding this comment.
With the refund counters gone in the latest revision, this helper returns exactly the same three collectors the pusher used to chain directly, and TestResourceGroupPushCollectors now just asserts the function body against itself. Suggest reverting both back to the original chained .Collector(...) calls to keep the diff minimal.
| consumptionRecordMap map[consumptionRecordKey]time.Time | ||
| // Records the latest activity time for metric series cleanup. This is | ||
| // independent from metering/actual consumption. | ||
| metricsActivityRecordMap map[metricsActivityRecordKey]time.Time |
There was a problem hiding this comment.
The consumptionRecordMap → metricsActivityRecordMap rename (~60 lines across manager/metrics/tests) was motivated by the earlier design where settlement-only reports also refreshed activity. With the settlement stream removed, every record here is once again driven by actual consumption, so the original name is accurate again. Suggest reverting the rename to shrink the diff.
| ruLabelType = tiflashTypeLabel | ||
| } | ||
| consumption := consumptionInfo.Consumption | ||
| if isZeroConsumption(consumption) { |
There was a problem hiding this comment.
This early return changes cleanup behavior for idle groups: previously an all-zero consumption report still refreshed the activity record, keeping the group's metric series alive indefinitely; now an idle-but-connected group's series get deleted after metricsCleanupTimeout and recreated on the next activity (a counter reset, which PromQL rate() tolerates). Probably fine — but is this intended? If kept, please call it out in the PR description. Same question for the guard in counterMetrics.add (L425).
| } | ||
|
|
||
| func (rm *ruMetering) add(consumption *consumptionItem) { | ||
| if consumption == nil || consumption.Consumption == nil { |
There was a problem hiding this comment.
This guard is unreachable: the only call path is backgroundMetricsFlush → ruCollector.Collect → rm.add, and the flush loop already skips items with consumptionInfo.Consumption == nil before calling Collect. Suggest dropping it — TestRUCollectorUsesConsumption still works as a plain behavior test.
| func reportedRequestConsumption(calculators []ResourceCalculator, req RequestInfo, tokenDelta *rmpb.Consumption) *rmpb.Consumption { | ||
| reported := cloneConsumption(tokenDelta) | ||
| adjustPagingPrechargeRRU(calculators, reported, req, -1) | ||
| sub(reported, writeReservationConsumption(calculators, req)) |
There was a problem hiding this comment.
Stripping the write reservation from the reported request consumption (and re-adding it at response time) is severable from paging pre-charge — the feature itself only needs the adjustPagingPrechargeRRU strip/restore pair. This line changes when WRU is reported for all writes (success: request time → response time), and it also fixes a real pre-existing quirk: a failed write straddling a report boundary used to over-count WRU, because the monotonic > guards in updateDeltaConsumption never subtract the payback. That fix is legitimate, but bundling it here widens the blast radius of an already-XXL PR. Could it move to a separate PR, or at least be called out explicitly in the description as an independent behavior change? If minimizing this PR is preferred, keeping the write path as-is would also let writeReservationConsumption/writeRefundConsumption (~40 lines) drop out.
There was a problem hiding this comment.
Agree. Extracted the bug and its fix to a separate issue / PR.
| delta.TiflashRUV2 = now.TiflashRUV2 - last.TiflashRUV2 | ||
| last.TiflashRUV2 = now.TiflashRUV2 | ||
| } | ||
| if isZeroConsumption(delta) { |
There was a problem hiding this comment.
Returning nil for an all-zero delta means new clients stop sending empty periodic reports. Old servers handle a nil ConsumptionSinceLastRequest fine (the flush loop already skips it), but combined with the server-side zero guard this is what changes the idle-group metric cleanup semantics — see my comment in metrics.go. Intended? Worth a line in the PR description either way.
| }) | ||
| delta := (new - counter.avgRUPerSecLastRU) / deltaDuration.Seconds() | ||
| ruDelta := new - counter.avgRUPerSecLastRU | ||
| if ruDelta < 0 { |
There was a problem hiding this comment.
This clamp also affects non-paging flows, and in the current revision I'm not sure any steady-state path still produces a negative delta: pre-charge refunds no longer enter gc.mu.consumption (the reported deltas are all non-negative), and the failed-write payback no longer decreases it either. The remaining source seems to be the transient add-then-sub window in onRequestWaitImpl's failure path, which pre-dates this PR. Is the clamp load-bearing for pre-charge, or defensive? A short code comment naming the path that produces negative deltas would help future readers.
Signed-off-by: Yuhao Zhang <yhzhang00@outlook.com>
Signed-off-by: Yuhao Zhang <yhzhang00@outlook.com>
Signed-off-by: Yuhao Zhang <yhzhang00@outlook.com>
Signed-off-by: Yuhao Zhang <yhzhang00@outlook.com>
Signed-off-by: Yuhao Zhang <yhzhang00@outlook.com>
| counter := gc.run.requestUnitTokens | ||
| if v := getRUValueFromConsumption(delta); v > 0 { | ||
| counter.limiter.RemoveTokens(time.Now(), v) | ||
| } else if v < 0 { |
There was a problem hiding this comment.
This branch catches more than the paging over-estimate the comment describes. A failed write's settlement delta is readBytesCost − writeCost, which is usually negative, so failed writes now refund their reserved write tokens to the local limiter through this path. On master, acquireTokens only acts on v > 0 and this payback was silently dropped, so this is a real behavior change for non-paging traffic: the net local debit for a failed write becomes just the read-bytes cost instead of the full write reservation. I believe the new behavior is the more correct one, but please: (1) update this comment (and the twin branch in onResponseWaitImpl) to say it covers both paging over-estimates and failed-write paybacks; (2) add a limiter-level assertion for the failed-write refund — TestFailedWriteReservationDoesNotEnterReportedConsumption only checks the reporting side; (3) document this together with the write-reservation reporting shift (WRU moving from request time to response time) in the PR description, which currently only describes the read path.
| } | ||
| } | ||
|
|
||
| func writeReservationConsumption(calculators []ResourceCalculator, req RequestInfo) *rmpb.Consumption { |
There was a problem hiding this comment.
Hot-path allocations: reportedRequestConsumption/reportedResponseConsumption run for every KV request, and this helper allocates the empty &rmpb.Consumption{} before the IsWrite() check, so pure-read workloads pay for it on both the request and response sides (same in writeRefundConsumption), on top of the unconditional cloneConsumption. add/sub are already nil-safe, so returning nil for non-writes removes most of it — roughly 4 extra allocations per read request back to master's level. Optionally, when the request is neither a write nor carries a paging hint, the clone can be skipped entirely and tokenDelta returned as-is (neither value is mutated afterwards), but the nil-for-non-writes fix alone is the important part.
Signed-off-by: Yuhao Zhang <yhzhang00@outlook.com>
Signed-off-by: Yuhao Zhang <yhzhang00@outlook.com>
Signed-off-by: Yuhao Zhang <yhzhang00@outlook.com>
Signed-off-by: Yuhao Zhang <yhzhang00@outlook.com>
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
client/resource_group/controller/group_controller.go (1)
453-459: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winClose the previous
cancelChbefore overwriting it.Line 455 stops the old timer, but Line 459 replaces
cancelChwithout waking any goroutine already waiting on the old channel. UseresetCounterNotifyLockedand close the returned channel after unlocking.Proposed fix
counter.notify.mu.Lock() - if counter.notify.setupNotificationTimer != nil { - counter.notify.setupNotificationTimer.Stop() - } + cancelCh := resetCounterNotifyLocked(counter) counter.notify.setupNotificationTimer = time.NewTimer(timerDuration) counter.notify.setupNotificationCh = counter.notify.setupNotificationTimer.C counter.notify.cancelCh = make(chan struct{}) counter.notify.setupNotificationThreshold = 1 counter.notify.mu.Unlock() + if cancelCh != nil { + close(cancelCh) + }As per coding guidelines, “Prevent goroutine leaks: pair with cancellation; consider errgroup.”
🤖 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 453 - 459, Close the existing cancelCh before assigning a new one in the notification reset path so any goroutine waiting on the old channel is unblocked; update the logic around resetCounterNotifyLocked/counter.notify.setupNotificationTimer to stop the old timer, capture the previous cancelCh, overwrite the fields, then close the old channel after releasing the lock to avoid leaking waiters.Source: Coding guidelines
🤖 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.
Outside diff comments:
In `@client/resource_group/controller/group_controller.go`:
- Around line 453-459: Close the existing cancelCh before assigning a new one in
the notification reset path so any goroutine waiting on the old channel is
unblocked; update the logic around
resetCounterNotifyLocked/counter.notify.setupNotificationTimer to stop the old
timer, capture the previous cancelCh, overwrite the fields, then close the old
channel after releasing the lock to avoid leaking waiters.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: e4ff3969-5934-42cb-af0a-c2d84c8b5c44
📒 Files selected for processing (16)
client/go.modclient/resource_group/controller/global_controller.goclient/resource_group/controller/group_controller.goclient/resource_group/controller/group_controller_test.goclient/resource_group/controller/limiter.goclient/resource_group/controller/limiter_test.goclient/resource_group/controller/metrics/metrics.goclient/resource_group/controller/model.goclient/resource_group/controller/model_test.goclient/resource_group/controller/testutil.gopkg/mcs/resourcemanager/server/manager_test.gopkg/mcs/resourcemanager/server/metering_test.gopkg/mcs/resourcemanager/server/metrics.gopkg/mcs/resourcemanager/server/metrics_test.gopkg/metering/utils.gopkg/metering/utils_test.go
✅ Files skipped from review due to trivial changes (1)
- pkg/metering/utils_test.go
🚧 Files skipped from review as they are similar to previous changes (3)
- client/resource_group/controller/limiter_test.go
- client/resource_group/controller/limiter.go
- client/resource_group/controller/testutil.go
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: JmPotato, rleungx The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
/unhold |
ti-chi-bot
Bot
removed
the
do-not-merge/hold
|
/test pull-unit-test-next-gen-2 |
|
@YuhaoZhang00: 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. |
…es hint (tikv#10611) close tikv#10612, closes tikv#10985\n\nSigned-off-by: Yuhao Zhang <yhzhang00@outlook.com> (cherry picked from commit 4e05b9d) Signed-off-by: JmPotato <github@ipotato.me>
3 participants
What problem does this PR solve?
Issue Number: close #10612
Under RC (resource control), paging coprocessor requests are today billed only at response settlement. A misbehaving resource group can therefore burst large reads before its token bucket reacts, because no RU is reserved up front. This PR introduces a pre-charge mechanism driven by a caller-supplied read-bytes prediction so the token bucket is pressured before the KV RPC is sent, while keeping the final bill accurate at settlement.
What is changed and how does it work?
The main logic is in
client/resource_group/controller, with matching resource-manager server-side metrics/metering handling.Pre-charge on request -
BeforeKVRequestreads an optionalPredictedReadBytes()provider fromRequestInfo. The hint is used only when the request is a read coprocessor request (IsCop() == trueandIsWrite() == false). When the hint is positive,KVCalculatoraddsReadBytesCost * predictedtodelta.RRU, and the existing token acquisition path reserves that many tokens before the KV RPC is sent. Writes, non-coprocessor reads, and coprocessor reads without a positive hint keep the existing settlement-only behavior.Refund on settlement -
AfterKVRequestsettles the pre-charge by subtracting the predicted basis and addingReadBytesCost * actual. The resulting token delta can be negative when the estimate was too high, soLimiter.RefundTokensis used as the inverse ofRemoveTokens. Reported consumption strips the synthetic pre-charge so pre-charged reads still report the same actual request consumption as before: request-side read base cost, then response-side actual read bytes and CPU.Consumption reporting - The synthetic paging pre-charge/refund delta is used only for local token accounting. It is stripped from
ConsumptionSinceLastRequest, so the resource-manager server still receives the same actual consumption as before. Empty consumption deltas are still reported as emptyConsumptionmessages, preserving the existing metric activity refresh behavior; nil consumption is ignored defensively on the server side.Observability and metering safeguards - five per-resource-group paging metrics are added, grouped by sampling unit. Server-side metering now also clamps negative RU values to zero before converting them to unsigned metering values, so malformed or defensive negative RU input cannot become an extremely large unsigned value.
Count:
cop_read_precharge_total- read coprocessor RPCs that carried a positivePredictedReadByteshint and triggered request-side read-byte pre-chargecop_read_no_precharge_total- read coprocessor RPCs that reached the controller without a positivePredictedReadByteshint, so request-side read-byte pre-charge was skippedBytes:
paging_precharge_bytes_total- sum of predicted hints used as the pre-charge basispaging_actual_bytes_total- actual bytes read by pre-charged RPCspaging_prediction_residual_bytes- distribution of signedactual - predictedfor pre-charged RPCsPre-charge is strictly opt-in: a client must expose
PredictedReadBytes()and pass a non-zero value on a read coprocessor request. Passing 0, omitting the optional provider, or using any write/non-coprocessor request preserves the existing settlement-only path. The wire protocol is unchanged.Related PRs
Check List
Tests
TestPredictedReadBytesPreCharge/TestPredictedReadBytesRequiresCopRead/TestRequestInfoPagingProvidersAreOptional/TestRequestInfoMissingPredictionProviderReturnsZeroHint/TestNoPreChargeWithoutPredictedReadBytes- request-side pre-charge gatingTestPagingPreChargeTokenRefund/TestPagingPreChargeNoRefundWhenActualExceedsEstimate/TestOnResponseImplPagingRefund/TestOnResponseWaitPrechargedPositiveSettlementDebitsImmediately/TestPagingPreChargeRefundOnFailedRead- response-side settlement and refundTestPagingPrechargeDoesNotEnterReportedRequestConsumption/TestPagingPrechargeRefundDoesNotEnterReportedResponseConsumption/TestReportedConsumptionStripsPagingPrecharge/TestReportedConsumptionRestoresPagingActualUsage- reported consumption excludes synthetic pre-chargeTestDeletePagingLabelsResetsSeries/TestCopReadNoPrechargeGatedByIsCop/TestCopReadNoPrechargeCounterObservedOnRequest/TestPagingPrechargeNotObservedOnThrottle- paging metric behaviorTestUpdateDeltaConsumptionReportsEmptyConsumption/TestRecordConsumptionKeepsRecordForEmptyConsumption/TestRecordConsumptionIgnoresNilConsumption/TestRUCollectorUsesConsumption/TestNewRUValueClampsNegativeValues- consumption reporting, server metrics, and metering handlingTestFailedWriteDoesNotUsePagingRefundPath/TestFailedWriteOnResponseDoesNotUsePagingRefundPath- split boundary for the failed-write accounting fixTestRefundTokens- limiter primitiveSide effects
RefundTokens; this affects only the local token bucket and does not enter reported consumption.Release note
Summary by CodeRabbit
New Features
Bug Fixes
Tests