Skip to content

store/copr: add RC paging pre-charge EMA and PredictedReadBytes hint#67941

Merged
ti-chi-bot[bot] merged 31 commits into
pingcap:masterfrom
YuhaoZhang00:feat/rc-paging-precharge
Jul 9, 2026
Merged

store/copr: add RC paging pre-charge EMA and PredictedReadBytes hint#67941
ti-chi-bot[bot] merged 31 commits into
pingcap:masterfrom
YuhaoZhang00:feat/rc-paging-precharge

Conversation

@YuhaoZhang00

@YuhaoZhang00 YuhaoZhang00 commented Apr 21, 2026

Copy link
Copy Markdown
Contributor

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?

  • New pkg/store/copr/ema.go: a time-aware EMA (tau=1s, alpha = 1 - exp(-delta/tau)) with Observe(bytes, now) / Predict(). BuildCopIterator seeds it with req.Paging.PagingSizeBytes, so byte-budget paging starts from the page-size prediction; row-count-only paging starts at 0 until the first observed page.
  • copIterator owns a single *ruEMA shared across its workers, so samples from the same logical scan feed one estimate.
  • Observation point: handleCopPagingResult observes positive pagingResponseReadBytes(resp) samples, where the helper returns ExecDetailsV2.ScanDetailV2.ProcessedVersionsSize, the same read-byte basis PD charges for classic.
  • Send side: in handleTaskOnce, TiDB sets tikvrpc.Request.PredictedReadBytes for row-count paging tasks and byte-budget paging requests (PagingSizeBytes > 0, even when task.paging is 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.
  • Grafana: new "Paging Pre-charge" row in pkg/metrics/grafana/tidb_resource_control.json / .jsonnet that 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 Pre-charge Coverage - rates of cop_read_precharge_total vs cop_read_no_precharge_total over read coprocessor RPCs, split by whether the request carried a positive PredictedReadBytes hint.
  • Paging Bytes - rates of paging_precharge_bytes_total and paging_actual_bytes_total for pre-charged coprocessor reads.
  • Paging Prediction Residual Bytes - p1/p10/p50/p90/p99 quantiles of paging_prediction_residual_bytes (signed actual - predicted), used to inspect predictor drift.

Note: The Grafana screenshot below is outdated and kept only for historical context.

image

Related PRs

Upstream support has merged:

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:

Companion PRs

Byte-budget paging prerequisites have merged:

Check List

Tests

  • Unit test
    • TestRUEMA* in pkg/store/copr/ema_test.go (seeded/unseeded prediction, shift/gap behavior, read-bytes helper, concurrency, non-monotonic time)
    • TestBuildCopTasksWithPagingSizeBytes covers byte-budget hints when task.paging is false; related tests cover request-builder and copr-cache behavior.
  • Integration test
  • Manual test - see below
  • No need to test

Manual test

Classic setup:

  • Cluster: 2 TiDB / 3 PD / 6 TiKV, single dh_app_280.dh_message_setting table (~8M rows)
  • Workload: 48 concurrent threads x 2 qps, full table scan
  • Resource group new_biz_rg set to RU_PER_SEC = 5000

(1) RU enforcement holds: the new_biz_rg consumption stays at the configured 5000 RU/s ceiling when paging_size_bytes is positive.

image

(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.

image

The chart shows the per-RPC absolute residual |actual_read_bytes - predicted_read_bytes| for each paging_size_bytes setting: left = under-charge (predicted < actual), right = over-charge (predicted > actual). Box bodies span the 5th-95th percentile.

TiDB X setup:

  • Cluster: Premium / TiDB X cse-multi, 2 TiDB / 3 PD / 3 TiKV + 3 tikv-worker, single dh_app_280.dh_message_setting table (~8M rows)
  • Workload: 48 concurrent threads x 2 qps/thread, full table scan
  • Resource limit: keyspace keyspace1 service_limit = 5000 RCU

(1) RU enforcement holds: the consumption stays at the configured 5000 RU/s ceiling when paging_size_bytes is positive.

image

Side effects

  • Performance regression: Consumes more CPU (internal tests confirmed no measurable extra CPU overhead)
  • Performance regression: Consumes more Memory
  • Breaking backward compatibility

Documentation

  • Affects user behaviors
  • Contains syntax changes
  • Contains variable changes
  • Contains experimental features
  • Changes MySQL compatibility

Release note

Resource control now pre-charges paging coprocessor reads with predicted read bytes before dispatch, tightening RU throttling for scans split across many paging RPCs. The Resource Control Grafana dashboard adds a "Paging Pre-charge" row.

Summary by CodeRabbit

  • New Features

    • EMA-driven prediction for read-byte estimation in coprocessor paging to improve resource forecasting.
    • New Grafana "Paging Pre-charge" row with panels for request counts, bytes, and prediction residuals.
  • Tests

    • Added comprehensive tests for EMA behavior: convergence, time responsiveness, concurrency, and paging byte accounting.
  • Chores

    • Updated build configuration and dependency references to enable the above changes.

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>
@ti-chi-bot ti-chi-bot Bot added do-not-merge/needs-linked-issue release-note-none Denotes a PR that doesn't merit a release note. labels Apr 21, 2026
@pantheon-ai

pantheon-ai Bot commented Apr 21, 2026

Copy link
Copy Markdown

@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.

@ti-chi-bot ti-chi-bot Bot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. contribution This PR is from a community contributor. needs-ok-to-test Indicates a PR created by contributors and need ORG member send '/ok-to-test' to start testing. labels Apr 21, 2026
@ti-chi-bot

ti-chi-bot Bot commented Apr 21, 2026

Copy link
Copy Markdown

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 /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Details

Instructions 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.

@tiprow

tiprow Bot commented Apr 21, 2026

Copy link
Copy Markdown

Hi @YuhaoZhang00. Thanks for your PR.

PRs from untrusted users cannot be marked as trusted with /ok-to-test in this repo meaning untrusted PR authors can never trigger tests themselves. Collaborators can still trigger tests on the PR using /test all.

I understand the commands that are listed here.

Details

Instructions 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.

@coderabbitai

coderabbitai Bot commented Apr 21, 2026

Copy link
Copy Markdown

Note

Reviews paused

It 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 reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

  • @coderabbitai resume to resume automatic reviews.
  • @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

  • ▶️ Resume reviews
  • 🔍 Trigger review
📝 Walkthrough

Walkthrough

Added 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

Cohort / File(s) Summary
Build Configuration
pkg/store/copr/BUILD.bazel
Added ema.go and ema_test.go to library and test sources; added @com_github_pingcap_kvproto//pkg/kvrpcpb to test deps.
EMA Implementation
pkg/store/copr/ema.go
New concurrency-safe, time-decay EMA ruEMA with Observe(bytes, now) and Predict() using τ=1s and clamped dt for out-of-order times.
EMA Tests
pkg/store/copr/ema_test.go
New tests for initial state, convergence, responsiveness to time gaps, helper pagingResponseReadBytes, concurrency safety, and non-monotonic timestamps.
Coprocessor Integration
pkg/store/copr/coprocessor.go
Injected ema *ruEMA into copIterator/workers; set tikvrpc.PredictedReadBytes = worker.ema.Predict() and call worker.ema.Observe(...) after paging responses; added pagingResponseReadBytes helper.
Dependency Management
go.mod
Added replace directives pointing github.com/tikv/client-go/v2 and github.com/tikv/pd/client to specific branch commits.
Observability
pkg/metrics/grafana/tidb_resource_control.json
Inserted a new "Paging Pre-charge" row with five panels (paging counts, bytes, residuals, RU, settlement delta) into the dashboard JSON.

Sequence Diagram

sequenceDiagram
    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
Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Suggested labels

ok-to-test

Suggested reviewers

  • nolouch
  • hawkingrei
  • zimulala

Poem

🐰 In tunnels of bytes I quietly creep,

I measure the reads while others sleep.
With decay and a hop I forecast the stream,
Predictions that nudge the paging machine.
Hooray for small hops that make throughput sing!

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 28.57% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Title check ✅ Passed The title accurately summarizes the main change: adding an RC paging pre-charge EMA and PredictedReadBytes hint.
Description check ✅ Passed The description follows the template and includes the issue number, problem summary, changes, tests, side effects, documentation, and release note.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

📥 Commits

Reviewing files that changed from the base of the PR and between ca3c902 and 51b5081.

📒 Files selected for processing (7)
  • pkg/metrics/distsql.go
  • pkg/metrics/metrics.go
  • pkg/store/copr/BUILD.bazel
  • pkg/store/copr/coprocessor.go
  • pkg/store/copr/ema.go
  • pkg/store/copr/ema_test.go
  • pkg/store/copr/metrics/metrics.go

Comment thread pkg/store/copr/coprocessor.go
Comment thread pkg/store/copr/ema.go Outdated
Comment thread pkg/metrics/distsql.go Outdated
Buckets: prometheus.ExponentialBuckets(1, 2, 10),
}, []string{LblStore})

DistSQLCoprEMASend = metricscommon.NewCounterVec(

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This naming is a bit abstract. Could we use a more intuitive name directly? For example, DistSQLCoprPredictReqBodySize, corresponding to the one above.

Comment thread pkg/store/copr/metrics/metrics.go Outdated
// 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

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ditto. Better not including EMA in the naming.

Comment thread pkg/store/copr/ema.go Outdated
Comment on lines +71 to +76
// 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
}

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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>
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>
@ti-chi-bot ti-chi-bot Bot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Apr 22, 2026

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 lastObsAt being the zero time.Time so the first Observe produces a huge dt, saturating alpha to 1 and seeding value directly from the first sample. This is intentional (per the PR description), but it's non-obvious and fragile — a well-meaning future change that initializes lastObsAt = time.Now() in newRUEMA would 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/the Observe alpha 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

📥 Commits

Reviewing files that changed from the base of the PR and between b0db938 and 119114a.

📒 Files selected for processing (6)
  • pkg/metrics/distsql.go
  • pkg/metrics/metrics.go
  • pkg/store/copr/coprocessor.go
  • pkg/store/copr/ema.go
  • pkg/store/copr/ema_test.go
  • pkg/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

Comment thread pkg/store/copr/coprocessor.go Outdated
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>
@YuhaoZhang00 YuhaoZhang00 requested a review from JmPotato April 22, 2026 06:36

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

♻️ Duplicate comments (2)
pkg/store/copr/coprocessor.go (2)

1701-1701: ⚠️ Potential issue | 🟠 Major

Only attach PredictedReadBytes when 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 | 🟡 Minor

Preserve real zero-byte samples instead of treating them as missing stats.

pagingResponseReadBytes returns 0 for both missing ScanDetailV2 and 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

📥 Commits

Reviewing files that changed from the base of the PR and between 119114a and 20f27fd.

📒 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>
@ti-chi-bot ti-chi-bot Bot removed the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Apr 22, 2026
@YuhaoZhang00 YuhaoZhang00 force-pushed the feat/rc-paging-precharge branch from cf6d5aa to 5c6dfa4 Compare July 9, 2026 01:16
@YuhaoZhang00 YuhaoZhang00 requested a review from JmPotato July 9, 2026 01:50
return 0
}
if scanDetail := pbResp.GetExecDetailsV2().GetScanDetailV2(); scanDetail != nil {
return scanDetail.GetProcessedVersionsSize()

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 JmPotato left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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="") =

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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.

Comment thread go.mod Outdated
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

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: stray blank line.

Comment thread pkg/store/copr/ema.go Outdated
func (e *ruEMA) Predict() uint64 {
e.mu.Lock()
defer e.mu.Unlock()
if e.value < 0 {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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())

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@YuhaoZhang00 YuhaoZhang00 requested review from JmPotato and rleungx July 9, 2026 04:13
Signed-off-by: Yuhao Zhang <yhzhang00@outlook.com>
@ti-chi-bot ti-chi-bot Bot added the needs-1-more-lgtm Indicates a PR needs 1 more LGTM. label Jul 9, 2026
@ti-chi-bot ti-chi-bot Bot added lgtm and removed needs-1-more-lgtm Indicates a PR needs 1 more LGTM. labels Jul 9, 2026
@ti-chi-bot

ti-chi-bot Bot commented Jul 9, 2026

Copy link
Copy Markdown

[LGTM Timeline notifier]

Timeline:

  • 2026-07-09 05:51:20.42170008 +0000 UTC m=+260866.457795146: ☑️ agreed by rleungx.
  • 2026-07-09 06:41:21.358335649 +0000 UTC m=+263867.394430695: ☑️ agreed by JmPotato.

@codecov

codecov Bot commented Jul 9, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 46.96970% with 35 lines in your changes missing coverage. Please review.
✅ Project coverage is 75.9019%. Comparing base (b6db59b) to head (1cdcf6e).
⚠️ Report is 4 commits behind head on master.

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     
Flag Coverage Δ
integration 45.8097% <46.9697%> (+6.1045%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Components Coverage Δ
dumpling 60.4471% <ø> (ø)
parser ∅ <ø> (∅)
br 64.0557% <ø> (+1.3343%) ⬆️
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nextgen also have such as file, do we need to change it too

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment thread pkg/store/copr/ema.go

// defaultRUEMATau is the decay time constant τ: sample weight drops to
// 1/e after τ.
const defaultRUEMATau = time.Second

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess EMA means exponential moving average, please comment this abbr

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in 1cdcf6e: expanded the ruEMA comment to spell out exponential moving average (EMA).

Signed-off-by: Yuhao Zhang <yhzhang00@outlook.com>
@YuhaoZhang00 YuhaoZhang00 requested a review from D3Hunter July 9, 2026 08:07

@D3Hunter D3Hunter left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/approve

@ti-chi-bot

ti-chi-bot Bot commented Jul 9, 2026

Copy link
Copy Markdown

[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

Details Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@ti-chi-bot ti-chi-bot Bot added the approved label Jul 9, 2026
@ti-chi-bot ti-chi-bot Bot merged commit ab7d93b into pingcap:master Jul 9, 2026
35 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

approved contribution This PR is from a community contributor. lgtm needs-ok-to-test Indicates a PR created by contributors and need ORG member send '/ok-to-test' to start testing. release-note Denotes a PR that will be considered when it comes time to generate release notes. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

coprocessor: add RC paging pre-charge read-bytes prediction (EMA + hint emission)

6 participants