store/copr: add RC paging pre-charge EMA and PredictedReadBytes hint#67941
Conversation
Maintain a time-aware EMA of actual per-RPC MVCC read bytes per logical
coprocessor scan (copIterator) and, once enough samples are in, pass
the prediction to PD via tikvrpc.Request.PredictedReadBytes so paging
pre-charge uses the learned estimate. Before readiness the hint is left
at 0 and PD skips pre-charge for that RPC, billing by actual read bytes
only in AfterKVRequest.
Observation point is handleCopPagingResult reading ScanDetailV2's
ProcessedVersionsSize (mirroring client-go's resourcecontrol.MakeResponseInfo
so the TiDB-side EMA and PD billing see the same byte quantity). The EMA
is stored on copIterator and shared via a pointer in each worker, so
multiple workers feeding the same logical scan converge on a single
estimate; an internal mutex serializes Observe/Predict.
Add tidb_distsql_copr_ema_send_total{type=cold|ready} incremented at hint
dispatch (once per outgoing cop RPC). Paired with the PD-side
paging_precharge_total / paging_nonprecharge_total counters this gives
the end-to-end pre-charge coverage ratio.
Signed-off-by: Yuhao Zhang <yhzhang00@outlook.com>
|
@YuhaoZhang00 I've received your pull request and will start the review. I'll conduct a thorough review covering code quality, potential issues, and implementation details. ⏳ This process typically takes 10-30 minutes depending on the complexity of the changes. ℹ️ Learn more details on Pantheon AI. |
|
Hi @YuhaoZhang00. Thanks for your PR. I'm waiting for a pingcap 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. |
|
Hi @YuhaoZhang00. Thanks for your PR. PRs from untrusted users cannot be marked as trusted with 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. |
|
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:
📝 WalkthroughWalkthroughAdded a time-aware exponential moving average (ruEMA) for predicting coprocessor read bytes, integrated it into copIterator/workers to set PredictedReadBytes and observe responses, plus tests, BUILD additions, go.mod replace directives, and new Grafana panels for paging metrics. Changes
Sequence DiagramsequenceDiagram
participant Client
participant CopIter
participant Worker
participant EMA
participant Handler
Client->>CopIter: BuildCopIterator()
CopIter->>EMA: newRUEMA()
CopIter->>Worker: newCopIteratorWorker(ema)
loop for each paging request
Handler->>EMA: Predict()
EMA-->>Handler: predictedBytes
Handler->>Worker: set tikvrpc.PredictedReadBytes
Handler->>Worker: send request
Worker-->>Handler: receive paging response
Handler->>EMA: Observe(readBytes, now)
EMA->>EMA: update value (alpha * delta)
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/store/copr/coprocessor.go`:
- Around line 1875-1883: pagingResponseReadBytes currently returns 0 for both
"no ScanDetailV2" and a real zero-byte read, causing callers to ignore real zero
samples; change pagingResponseReadBytes to indicate presence of scan stats
(e.g., return a (bytes uint64, ok bool) or an optional) by inspecting
pbResp.GetExecDetailsV2().GetScanDetailV2(), keep GetProcessedVersionsSize() for
the bytes value, and update the caller that currently drops zero samples (the
code that invokes pagingResponseReadBytes and filters out zero results) to only
skip samples when ok is false (missing stats) but still feed real zero-byte
values into the EMA.
In `@pkg/store/copr/ema.go`:
- Around line 59-68: The code currently sets e.lastObsAt = now unconditionally
which rewinds the timestamp on out-of-order observations; change Observe so that
you compute dt and update e.value as now does, but only assign e.lastObsAt = now
if now.After(e.lastObsAt) (i.e., now > e.lastObsAt) to avoid moving the clock
backward; keep incrementing e.samples and other logic the same so late
observations still affect the EMA but do not reduce future Δt calculations. Use
the existing symbols e.lastObsAt, dt, alpha, e.value, e.tau and e.samples to
locate and modify the logic.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 75e82508-d538-4e6b-ab84-c91c126b6fb5
📒 Files selected for processing (7)
pkg/metrics/distsql.gopkg/metrics/metrics.gopkg/store/copr/BUILD.bazelpkg/store/copr/coprocessor.gopkg/store/copr/ema.gopkg/store/copr/ema_test.gopkg/store/copr/metrics/metrics.go
| Buckets: prometheus.ExponentialBuckets(1, 2, 10), | ||
| }, []string{LblStore}) | ||
|
|
||
| DistSQLCoprEMASend = metricscommon.NewCounterVec( |
There was a problem hiding this comment.
This naming is a bit abstract. Could we use a more intuitive name directly? For example, DistSQLCoprPredictReqBodySize, corresponding to the one above.
| // skips pre-charge. Denominator is all cop RPC sends, so | ||
| // (EMASendCold + EMASendReady) is the authoritative "total read RPCs" | ||
| // basis for pre-charge coverage ratios at the TiDB side. | ||
| EMASendCold prometheus.Counter |
There was a problem hiding this comment.
Ditto. Better not including EMA in the naming.
| // IsReady reports whether enough samples have been seen to trust Predict. | ||
| func (e *ruEMA) IsReady() bool { | ||
| e.mu.Lock() | ||
| defer e.mu.Unlock() | ||
| return e.samples >= ruEMAMinSamples | ||
| } |
There was a problem hiding this comment.
Why introduce a state called "cold" or "not ready"? Can we just return the latest sampled value directly, regardless of what it is? After all, in our case, deducting something is better than deducting nothing at all, right?
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>
cc6eb6b to
b0db938
Compare
Signed-off-by: Yuhao Zhang <yhzhang00@outlook.com>
Signed-off-by: Yuhao Zhang <yhzhang00@outlook.com>
…formula Rely on a huge initial dt (zero lastObsAt) to saturate alpha to 1 on the first Observe, seeding value without a special-case branch. Also guard lastObsAt against rewinds on out-of-order concurrent Observes. Signed-off-by: Yuhao Zhang <yhzhang00@outlook.com>
Replace DistSQLCoprEMASend / EMASendCold / EMASendReady with DistSQLCoprPredictHintSend / PredictHintAbsent / PredictHintAttached. The metric name now describes the user-visible property (whether a PredictedReadBytes hint was attached to the RPC) rather than the internal EMA mechanism. Signed-off-by: Yuhao Zhang <yhzhang00@outlook.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
pkg/store/copr/ema.go (1)
36-53: Document the zero-time seeding invariant.The cold-start behavior relies on
lastObsAtbeing the zerotime.Timeso the firstObserveproduces a hugedt, saturatingalphato 1 and seedingvaluedirectly from the first sample. This is intentional (per the PR description), but it's non-obvious and fragile — a well-meaning future change that initializeslastObsAt = time.Now()innewRUEMAwould silently break seeding (first sample would get alpha ≈ 0 and the EMA would stay at 0 for a long time).A brief comment on
newRUEMA/theObservealpha line stating the invariant would protect the behavior.As per coding guidelines: "Comments SHOULD explain non-obvious intent, constraints, invariants, concurrency guarantees..."
🛠️ Suggested comment
func newRUEMA() *ruEMA { + // lastObsAt is intentionally left as the zero time so that the first + // Observe sees dt >> tau and alpha ≈ 1, seeding value from the first + // sample. Do not initialize lastObsAt here. return &ruEMA{tau: defaultRUEMATau} }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/store/copr/ema.go` around lines 36 - 53, Document the zero-time seeding invariant: add a brief comment in newRUEMA and next to the alpha calculation in Observe explaining that lastObsAt must start as the zero time so the very first Observe produces a very large dt (alpha ≈ 1) and seeds ruEMA.value from the first sample; note that initializing lastObsAt = time.Now() would prevent seeding (alpha ≈ 0) and break cold-start behavior. Reference the ruEMA struct field lastObsAt, the constructor newRUEMA, the Observe method, and the alpha = 1 - math.Exp(...) line when adding the comment.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/store/copr/coprocessor.go`:
- Around line 1701-1706: The PredictHint counters are being updated for every
coprocessor RPC even when paging is not applicable; modify handleTaskOnce so you
only call worker.ema.Predict() and touch req.PredictedReadBytes /
copr_metrics.PredictHintAttached/Absent when the request can actually use paging
(check task.paging or worker.req.Paging.Enable and skip for non-paging store
types or when paging was disabled by buildCopTasks), i.e., gate the Predict()
invocation and the two metric increments behind that paging condition so only
paging-capable RPCs affect the metrics and avoid the extra hot-path work.
---
Nitpick comments:
In `@pkg/store/copr/ema.go`:
- Around line 36-53: Document the zero-time seeding invariant: add a brief
comment in newRUEMA and next to the alpha calculation in Observe explaining that
lastObsAt must start as the zero time so the very first Observe produces a very
large dt (alpha ≈ 1) and seeds ruEMA.value from the first sample; note that
initializing lastObsAt = time.Now() would prevent seeding (alpha ≈ 0) and break
cold-start behavior. Reference the ruEMA struct field lastObsAt, the constructor
newRUEMA, the Observe method, and the alpha = 1 - math.Exp(...) line when adding
the comment.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: d47ec562-c505-4865-92da-f0ddf80dddaa
📒 Files selected for processing (6)
pkg/metrics/distsql.gopkg/metrics/metrics.gopkg/store/copr/coprocessor.gopkg/store/copr/ema.gopkg/store/copr/ema_test.gopkg/store/copr/metrics/metrics.go
✅ Files skipped from review due to trivial changes (1)
- pkg/metrics/distsql.go
🚧 Files skipped from review as they are similar to previous changes (2)
- pkg/store/copr/metrics/metrics.go
- pkg/store/copr/ema_test.go
Removed DistSQLCoprPredictHintSend and its PredictHintAbsent / PredictHintAttached shorthands. The same events are already counted on the PD side (paging_precharge_total / paging_nonprecharge_total) per resource group; a client-side mirror adds only debug-time signal at the cost of a second registration surface, and cannot be compared to the PD counters without manual per-RG aggregation. Signed-off-by: Yuhao Zhang <yhzhang00@outlook.com>
There was a problem hiding this comment.
♻️ Duplicate comments (2)
pkg/store/copr/coprocessor.go (2)
1701-1701:⚠️ Potential issue | 🟠 MajorOnly attach
PredictedReadByteswhen the paging EMA is ready.Line 1701 bypasses the
IsReady()contract, so the first observed page can immediately seed a cold-start pre-charge hint. Keep the hint scoped to paging tasks and require EMA readiness before setting the field.🐛 Proposed fix
- req.PredictedReadBytes = worker.ema.Predict() + if task.paging && worker.ema.IsReady() { + req.PredictedReadBytes = worker.ema.Predict() + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/store/copr/coprocessor.go` at line 1701, Only set req.PredictedReadBytes unconditionally — guard it so the EMA cold-start doesn't seed a pre-charge hint: check worker.ema.IsReady() and that the request is a paging task before calling worker.ema.Predict(); e.g. replace the direct assignment of req.PredictedReadBytes = worker.ema.Predict() with a conditional that verifies worker.ema.IsReady() and that the current request is a paging request (use the request/task paging flag or helper used elsewhere in this file), then call worker.ema.Predict() and assign only in that branch.
1858-1866:⚠️ Potential issue | 🟡 MinorPreserve real zero-byte samples instead of treating them as missing stats.
pagingResponseReadBytesreturns0for both missingScanDetailV2and valid zero-byte reads, then Lines 1888-1890 drop zero samples. This can leave a stale positive estimate active for later empty pages.🐛 Proposed fix
-func pagingResponseReadBytes(pbResp *coprocessor.Response) uint64 { +func pagingResponseReadBytes(pbResp *coprocessor.Response) (uint64, bool) { if pbResp == nil { - return 0 + return 0, false } if scanDetail := pbResp.GetExecDetailsV2().GetScanDetailV2(); scanDetail != nil { - return scanDetail.GetProcessedVersionsSize() + return scanDetail.GetProcessedVersionsSize(), true } - return 0 + return 0, false } @@ - if readBytes := pagingResponseReadBytes(resp.pbResp); readBytes > 0 { + if readBytes, ok := pagingResponseReadBytes(resp.pbResp); ok { worker.ema.Observe(readBytes, time.Now()) }Also applies to: 1888-1890
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@pkg/store/copr/coprocessor.go`:
- Line 1701: Only set req.PredictedReadBytes unconditionally — guard it so the
EMA cold-start doesn't seed a pre-charge hint: check worker.ema.IsReady() and
that the request is a paging task before calling worker.ema.Predict(); e.g.
replace the direct assignment of req.PredictedReadBytes = worker.ema.Predict()
with a conditional that verifies worker.ema.IsReady() and that the current
request is a paging request (use the request/task paging flag or helper used
elsewhere in this file), then call worker.ema.Predict() and assign only in that
branch.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 495c99f0-d8f1-4708-a19f-664db3b2d327
📒 Files selected for processing (1)
pkg/store/copr/coprocessor.go
Expose the new PD-client paging observability counters and histograms via a dedicated "Paging Pre-charge" row in tidb_resource_control.json. Panels: request count, bytes rate, prediction residual bytes quantiles, RU rate, and settlement RU delta quantiles. Signed-off-by: Yuhao Zhang <yhzhang00@outlook.com>
Signed-off-by: Yuhao Zhang <yhzhang00@outlook.com>
cf6d5aa to
5c6dfa4
Compare
Signed-off-by: Yuhao Zhang <yhzhang00@outlook.com>
| return 0 | ||
| } | ||
| if scanDetail := pbResp.GetExecDetailsV2().GetScanDetailV2(); scanDetail != nil { | ||
| return scanDetail.GetProcessedVersionsSize() |
There was a problem hiding this comment.
In NextGen builds this learns a smaller byte value than PD later settles. client-go’s MakeResponseInfo uses max(TotalVersionsSize, ProcessedVersionsSize) when config.NextGen is true, but the EMA here always trains on ProcessedVersionsSize, so scans with many skipped MVCC versions will keep under-predicting and the RU debit still lands mostly at response settlement instead of request pre-charge.
JmPotato
left a comment
There was a problem hiding this comment.
One must-fix on the dashboard netting layer plus three small nits, all inline below. The EMA/hint implementation itself looks good — verified against the merged PD/client-go behavior and ran the copr tests locally with race.
| "sum(rate(" + metric + "{" + filters + "}[1m]))" | ||
| else | ||
| "sum(rate(" + metric + "{" + filters + "}[1m])) by (" + groupBy + ")"; | ||
| local netUnitRURate(unit, filters, groupBy="") = |
There was a problem hiding this comment.
[must-fix] resource_manager_resource_unit_*_request_unit_refund_sum does not exist in the merged PD — tikv/pd#10611 dropped the refund counters together with the settlement design during review. The or debit * 0 fallback keeps the panels rendering, but this whole netting layer subtracts a metric that is never emitted, and against the merged PD the plain _sum series is already net/actual. Please revert netUnitRURate/netRURate/clamp_min and the panel changes built on them (both jsonnets + regenerated JSONs). The new Paging Pre-charge row is unaffected and can stay.
| github.com/apache/arrow-go/v18 => github.com/joechenrh/arrow-go/v18 v18.0.0-20250911101656-62c34c9a3b82 | ||
| github.com/go-ldap/ldap/v3 => github.com/YangKeao/ldap/v3 v3.4.5-0.20230421065457-369a3bab1117 | ||
| github.com/pingcap/tidb/pkg/parser => ./pkg/parser | ||
|
|
| func (e *ruEMA) Predict() uint64 { | ||
| e.mu.Lock() | ||
| defer e.mu.Unlock() | ||
| if e.value < 0 { |
There was a problem hiding this comment.
nit: unreachable — value is a convex combination of the non-negative seed and non-negative samples, so it can never go negative. Fine to drop.
| } | ||
|
|
||
| if readBytes := pagingResponseReadBytes(resp.pbResp); readBytes > 0 { | ||
| worker.ema.Observe(readBytes, time.Now()) |
There was a problem hiding this comment.
nit: worth a one-line comment that the final page (pagingRange == nil returns above) is intentionally not observed, giving the EMA a slight conservative (high) bias.
Signed-off-by: Yuhao Zhang <yhzhang00@outlook.com>
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #67941 +/- ##
================================================
- Coverage 76.3227% 75.9019% -0.4209%
================================================
Files 2041 2079 +38
Lines 560306 580743 +20437
================================================
+ Hits 427641 440795 +13154
- Misses 131764 137712 +5948
- Partials 901 2236 +1335
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
nextgen also have such as file, do we need to change it too
There was a problem hiding this comment.
Fixed in 1cdcf6e: added the same Paging Pre-charge row to pkg/metrics/nextgengrafana/tidb_resource_control_with_keyspace_name.jsonnet and regenerated the corresponding JSON.
|
|
||
| // defaultRUEMATau is the decay time constant τ: sample weight drops to | ||
| // 1/e after τ. | ||
| const defaultRUEMATau = time.Second |
There was a problem hiding this comment.
I guess EMA means exponential moving average, please comment this abbr
There was a problem hiding this comment.
Fixed in 1cdcf6e: expanded the ruEMA comment to spell out exponential moving average (EMA).
Signed-off-by: Yuhao Zhang <yhzhang00@outlook.com>
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: cfzjywxk, D3Hunter, JmPotato, rleungx, XuHuaiyu 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 |
What problem does this PR solve?
Issue Number: close #68085
Problem Summary:
Under RC (resource control), paging coprocessor reads used to be billed only at response settlement. For scans split across many paging RPCs, throttling reacted only after the bytes had already been read. This PR adds a per-logical-scan read-bytes EMA in the cop iterator and sends the prediction to PD as
PredictedReadBytes, so PD can reserve tokens before dispatching the RPC.What changed and how does it work?
pkg/store/copr/ema.go: a time-aware EMA (tau=1s,alpha = 1 - exp(-delta/tau)) withObserve(bytes, now)/Predict().BuildCopIteratorseeds it withreq.Paging.PagingSizeBytes, so byte-budget paging starts from the page-size prediction; row-count-only paging starts at 0 until the first observed page.copIteratorowns a single*ruEMAshared across its workers, so samples from the same logical scan feed one estimate.handleCopPagingResultobserves positivepagingResponseReadBytes(resp)samples, where the helper returnsExecDetailsV2.ScanDetailV2.ProcessedVersionsSize, the same read-byte basis PD charges for classic.handleTaskOnce, TiDB setstikvrpc.Request.PredictedReadBytesfor row-count paging tasks and byte-budget paging requests (PagingSizeBytes > 0, even whentask.pagingis false). PD/client-go gates request-side pre-charge on read coprocessor RPCs with a positive hint; if the hint is 0, the request stays settlement-only.pkg/metrics/grafana/tidb_resource_control.json/.jsonnetthat surfaces cop-read pre-charge coverage and byte-oriented PD-client paging metrics from client/resource_group: add RC paging pre-charge with PredictedReadBytes hint tikv/pd#10611.Observability (Grafana row "Paging Pre-charge")
Updated on July 8, 2026: tikv/pd#10611 and tikv/client-go#1947 have merged. The row now uses the current PD-client
resource_manager_client_request_*metrics grouped into 3 panels.cop_read_precharge_totalvscop_read_no_precharge_totalover read coprocessor RPCs, split by whether the request carried a positivePredictedReadByteshint.paging_precharge_bytes_totalandpaging_actual_bytes_totalfor pre-charged coprocessor reads.paging_prediction_residual_bytes(signedactual - predicted), used to inspect predictor drift.Related PRs
Upstream support has merged:
PredictedReadByteshint to client-go/tikvrpc.Dependency sync note (same context as tikv/client-go#1947): the client-go/PD bump also carries merged PD client API changes, so this PR includes small TiDB mock/build adaptations for:
Watchnow returns*metastorage.WatchResponse.LoadKeyspaceByID(ctx, id).Companion PRs
Byte-budget paging prerequisites have merged:
tidb_paging_size_bytesand forwards it to coprocessor requests.paging_size_bytestocoprocessor.Request.Check List
Tests
TestRUEMA*inpkg/store/copr/ema_test.go(seeded/unseeded prediction, shift/gap behavior, read-bytes helper, concurrency, non-monotonic time)TestBuildCopTasksWithPagingSizeBytescovers byte-budget hints whentask.pagingis false; related tests cover request-builder and copr-cache behavior.Manual test
Classic setup:
dh_app_280.dh_message_settingtable (~8M rows)new_biz_rgset toRU_PER_SEC = 5000(1) RU enforcement holds: the
new_biz_rgconsumption stays at the configured 5000 RU/s ceiling whenpaging_size_bytesis positive.(2) EMA prediction accuracy: For >95% of paging RPCs the residual stays within 1% of
paging_size_bytes. The over-prediction tail comes from trailing partial-page RPCs at the end of a logical scan, where an EMA seeded on full-page samples necessarily overshoots - expected by design.TiDB X setup:
dh_app_280.dh_message_settingtable (~8M rows)keyspace1service_limit = 5000 RCU(1) RU enforcement holds: the consumption stays at the configured 5000 RU/s ceiling when
paging_size_bytesis positive.Side effects
Documentation
Release note
Summary by CodeRabbit
New Features
Tests
Chores