store/copr: add RC byte-budget paging framework#67504
store/copr: add RC byte-budget paging framework#67504JmPotato wants to merge 6 commits intopingcap:masterfrom
Conversation
When Resource Control is enabled, force-enable paging for eligible coprocessor requests and set a byte budget (paging_size_bytes) per page. This limits the scanned bytes of each page so that RU cost is bounded and pre-charged in Phase 1, preventing concurrent workers from all hitting Phase 2 simultaneously. Signed-off-by: JmPotato <github@ipotato.me>
Signed-off-by: JmPotato <github@ipotato.me>
…ation Test rcPagingEligible conditions (RC enabled, resource group, TiKV, DAG) and verify pagingSizeBytes is correctly propagated to copTasks and cleared when small limit disables paging. Signed-off-by: JmPotato <github@ipotato.me>
ti-chi-bot
bot
added
do-not-merge/needs-linked-issue
release-note-none
|
Review Complete Findings: 0 issues ℹ️ Learn more details on Pantheon AI. |
ti-chi-bot
bot
added
the
size/XXL
|
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:
📝 WalkthroughWalkthroughAdds Resource-Control (RC) byte-budget paging for TiKV DAG coprocessor requests, propagates a per-page byte budget through DistSQL/session/sysvar plumbing and cop task construction, introduces paging-size constants (256 KB–4 MB), updates go module pins and Bazel deps, adds tests and adjusts test sharding/deps. Changes
Sequence Diagram(s)sequenceDiagram
participant Planner as "BuildCopIterator\n(Planner)"
participant Builder as "buildCopTaskOpt\n/ copTask"
participant TiKV as "TiKV\n(Coprocessor)"
participant ResourceCtrl as "Resource Control\n(config)"
rect rgba(200,230,201,0.5)
Planner->>ResourceCtrl: check RC enabled & resource group
ResourceCtrl-->>Planner: RC enabled? (yes/no)
Planner->>Planner: rcPagingEligible(req)?
alt eligible
Planner->>Builder: set paging=true,\nrcPagingSizeBytes, min/max
Builder->>TiKV: send Request(Paging=true,\nPagingSizeBytes=rcPagingSizeBytes)
TiKV-->>Builder: paged response chunk
Builder-->>Planner: assemble remains task\n(propagate pagingSizeBytes)
else not eligible
Planner->>Builder: normal request (no RC paging)
Builder->>TiKV: send Request(Paging=false)
TiKV-->>Builder: full/non-paged response
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
JmPotato
force-pushed
the
demo/ru-paging-size
branch
from
1550f30 to
a2c0781
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
go.mod (1)
371-374: Document the temporary nature and release plan for forked dependencies.Since this PR implements a coordinated multi-repository feature (RC byte-budget paging), consider adding comments in go.mod to:
- Explain why these forks are necessary (awaiting upstream releases with paging_size_bytes support)
- Reference the corresponding upstream PRs/issues
- Document the plan for transitioning to official releases
This will help future maintainers understand the dependency strategy and track when these can be replaced.
📝 Proposed documentation addition
replace ( // Downgrade grpc to v1.63.2, as well as other related modules. 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 + // TODO: RC byte-budget paging feature - replace with official releases once available + // Tracking: pingcap/kvproto#1448 and related upstream changes in PD, client-go, TiKV github.com/pingcap/kvproto => github.com/JmPotato/kvproto v0.0.0-20260401045836-48e3f28d9c74 github.com/pingcap/tidb/pkg/parser => ./pkg/parser github.com/tikv/client-go/v2 => github.com/JmPotato/client-go/v2 v2.0.0-20260402023154-0eedbbb67070 github.com/tikv/pd/client => github.com/JmPotato/pd/client v0.0.0-20260402021839-b98ff48814c2🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@go.mod` around lines 371 - 374, Add explanatory comments in go.mod next to the replaced module lines (the github.com/pingcap/kvproto, github.com/tikv/client-go/v2, and github.com/tikv/pd/client replacements) stating these are temporary forks to obtain paging_size_bytes support, reference the upstream PRs or issue URLs that add paging_size_bytes, and include a brief transition plan (e.g., watch upstream PRs, replace forks once upstream releases include the feature and update require/replace lines). Ensure comments mention the intended owner/reviewer and a target to remove the forks once official releases are available.pkg/store/copr/coprocessor_test.go (1)
733-778: Exercise the RC wiring throughBuildCopIterator.This test seeds
rcPagingSizeBytesdirectly intobuildCopTasks, so it won't catch regressions in the actual feature wiring insideBuildCopIterator—especially the paging-vs-store-batch interaction. Please add a targeted case with RC enabled andStoreBatchSize > 0that asserts batching is shut off whilepagingSizeBytesstill propagates.Based on learnings: Applies to pkg/store/|pkg/kv/ : For storage behavior changes, perform targeted unit tests; use realtikv tests if behavior depends on real TiKV.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/store/copr/coprocessor_test.go` around lines 733 - 778, Add a test that exercises the RC wiring via BuildCopIterator instead of seeding rcPagingSizeBytes into buildCopTasks: create a BuildCopIterator (or call BuildCopIterator through the same code-path used by production) with RC enabled, set req.Paging as in the existing test, set StoreBatchSize > 0 on the iterator options, and set rcPagingSizeBytes to paging.MaxPagingSizeBytes; assert that the resulting iterator disables store batching (StoreBatchSize behavior or equivalent flag is off) while the iterator or its underlying cop-task still has paging == true and pagingSizeBytes == paging.MaxPagingSizeBytes; use the same helpers (e.g., buildCopRanges, NewRegionCache/pd client bootstrapping) and mirror assertions from TestBuildCopTasksWithRCPagingSizeBytes so regressions in the BuildCopIterator wiring are caught.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@go.mod`:
- Around line 371-374: go.mod currently redirects core modules
(github.com/pingcap/kvproto, github.com/tikv/client-go/v2,
github.com/tikv/pd/client) to a personal JmPotato fork which mismatches versions
in DEPS.bzl; to fix, restore these replace directives to the official upstream
module versions (or remove them), then regenerate DEPS.bzl by running make
bazel_prepare so Bazel and Go use the same versions; if the fork was
intentionally used for testing, document that fact in the PR and ensure you
coordinate upstream releases before merging so the temporary forked replaces are
removed prior to production.
In `@pkg/store/copr/coprocessor.go`:
- Around line 142-156: The code checks checkStoreBatchCopr(req) before
rcPagingEligible(req) can flip req.Paging.Enable, allowing RC-eligible DAG
requests to incorrectly keep StoreBatchSize > 0; after rcPagingEligible(req)
forces paging on, re-run the store-batch gate and clear batching when paging was
enabled (i.e., call checkStoreBatchCopr(req) or explicitly set
req.StoreBatchSize = 0 if req.Paging.Enable was turned on by rcPagingEligible)
so newBatchTaskBuilder never receives a request that has both paging forced by
RC and a positive StoreBatchSize; update the logic around checkStoreBatchCopr,
rcPagingEligible, req.Paging.Enable, and StoreBatchSize accordingly.
---
Nitpick comments:
In `@go.mod`:
- Around line 371-374: Add explanatory comments in go.mod next to the replaced
module lines (the github.com/pingcap/kvproto, github.com/tikv/client-go/v2, and
github.com/tikv/pd/client replacements) stating these are temporary forks to
obtain paging_size_bytes support, reference the upstream PRs or issue URLs that
add paging_size_bytes, and include a brief transition plan (e.g., watch upstream
PRs, replace forks once upstream releases include the feature and update
require/replace lines). Ensure comments mention the intended owner/reviewer and
a target to remove the forks once official releases are available.
In `@pkg/store/copr/coprocessor_test.go`:
- Around line 733-778: Add a test that exercises the RC wiring via
BuildCopIterator instead of seeding rcPagingSizeBytes into buildCopTasks: create
a BuildCopIterator (or call BuildCopIterator through the same code-path used by
production) with RC enabled, set req.Paging as in the existing test, set
StoreBatchSize > 0 on the iterator options, and set rcPagingSizeBytes to
paging.MaxPagingSizeBytes; assert that the resulting iterator disables store
batching (StoreBatchSize behavior or equivalent flag is off) while the iterator
or its underlying cop-task still has paging == true and pagingSizeBytes ==
paging.MaxPagingSizeBytes; use the same helpers (e.g., buildCopRanges,
NewRegionCache/pd client bootstrapping) and mirror assertions from
TestBuildCopTasksWithRCPagingSizeBytes so regressions in the BuildCopIterator
wiring are caught.
🪄 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: 99dd612b-2680-493e-8272-6b0fb21125db
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (4)
go.modpkg/store/copr/coprocessor.gopkg/store/copr/coprocessor_test.gopkg/util/paging/paging.go
| if !checkStoreBatchCopr(req) { | ||
| req.StoreBatchSize = 0 | ||
| } | ||
| // RC Paging: if Resource Control is enabled and the request targets a | ||
| // resource group on TiKV with a DAG type, force-enable paging and set | ||
| // a default byte budget so each page's scanned bytes are bounded. | ||
| rcPagingSizeBytes := uint64(0) | ||
| if rcPagingEligible(req) { | ||
| if !req.Paging.Enable { | ||
| req.Paging.Enable = true | ||
| req.Paging.MinPagingSize = paging.MinPagingSize | ||
| req.Paging.MaxPagingSize = paging.MinAllowedMaxPagingSize | ||
| } | ||
| rcPagingSizeBytes = paging.MaxPagingSizeBytes | ||
| } |
There was a problem hiding this comment.
Re-run the store-batch gate after RC forces paging.
Line 142 decides whether store batching stays enabled before Line 149 can flip req.Paging.Enable for RC. That lets an RC-eligible DAG request with StoreBatchSize > 0 still enter newBatchTaskBuilder, even though checkStoreBatchCopr explicitly treats paging and batching as mutually exclusive. The first build can then take the batching path while still carrying the new RC paging budget, which undermines the RU precharge behavior this change is trying to enforce.
💡 Suggested fix
- if !checkStoreBatchCopr(req) {
- req.StoreBatchSize = 0
- }
// RC Paging: if Resource Control is enabled and the request targets a
// resource group on TiKV with a DAG type, force-enable paging and set
// a default byte budget so each page's scanned bytes are bounded.
rcPagingSizeBytes := uint64(0)
if rcPagingEligible(req) {
if !req.Paging.Enable {
req.Paging.Enable = true
req.Paging.MinPagingSize = paging.MinPagingSize
req.Paging.MaxPagingSize = paging.MinAllowedMaxPagingSize
}
rcPagingSizeBytes = paging.MaxPagingSizeBytes
}
+ // Paging and store-batch are mutually exclusive. Re-evaluate after RC may
+ // have force-enabled paging.
+ if !checkStoreBatchCopr(req) {
+ req.StoreBatchSize = 0
+ }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/store/copr/coprocessor.go` around lines 142 - 156, The code checks
checkStoreBatchCopr(req) before rcPagingEligible(req) can flip
req.Paging.Enable, allowing RC-eligible DAG requests to incorrectly keep
StoreBatchSize > 0; after rcPagingEligible(req) forces paging on, re-run the
store-batch gate and clear batching when paging was enabled (i.e., call
checkStoreBatchCopr(req) or explicitly set req.StoreBatchSize = 0 if
req.Paging.Enable was turned on by rcPagingEligible) so newBatchTaskBuilder
never receives a request that has both paging forced by RC and a positive
StoreBatchSize; update the logic around checkStoreBatchCopr, rcPagingEligible,
req.Paging.Enable, and StoreBatchSize accordingly.
![pantheon-ai[bot]](https://avatars.githubusercontent.com/in/2265341?s=60&v=4){kind=small}
Signed-off-by: JmPotato <github@ipotato.me>
JmPotato
force-pushed
the
demo/ru-paging-size
branch
from
a2c0781 to
7a480de
Compare
Add a new session variable `tidb_rc_paging_size_bytes` to allow dynamically controlling the byte budget per page for RC paging via SQL. - Default: 4MB (paging.MaxPagingSizeBytes) - Set to 0 to disable byte-budget paging - Supports both GLOBAL and SESSION scope Usage: SET @@tidb_rc_paging_size_bytes = 4194304; -- 4MB (default) SET @@tidb_rc_paging_size_bytes = 1048576; -- 1MB SET @@tidb_rc_paging_size_bytes = 0; -- disable Signed-off-by: JmPotato <github@ipotato.me>
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
pkg/sessionctx/variable/sysvar.go (1)
2973-2974: UsetidbOptPositiveInt32to align with the pattern forMinPagingSizeandMaxPagingSize.Line 2974 uses
TidbOptInt64with an explicitint()cast, while the similar paging-size sysvars on lines 2966 and 2970 usetidbOptPositiveInt32withstrconv.Atoi. For consistency and clarity, apply the same safe helper:Suggested fix
{Scope: vardef.ScopeGlobal | vardef.ScopeSession, Name: vardef.TiDBRCPagingSizeBytes, Value: strconv.Itoa(vardef.DefRCPagingSizeBytes), Type: vardef.TypeUnsigned, MinValue: 0, MaxValue: math.MaxInt64, SetSession: func(s *SessionVars, val string) error { - s.RCPagingSizeBytes = int(TidbOptInt64(val, int64(vardef.DefRCPagingSizeBytes))) + s.RCPagingSizeBytes = tidbOptPositiveInt32(val, vardef.DefRCPagingSizeBytes) return nil }},🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/sessionctx/variable/sysvar.go` around lines 2973 - 2974, The TiDBRCPagingSizeBytes sysvar SetSession currently uses TidbOptInt64 with an int(...) cast; replace that with the same pattern used for MinPagingSize/MaxPagingSize by using tidbOptPositiveInt32 together with strconv.Atoi to parse/validate the value and assign to s.RCPagingSizeBytes, ensuring the helper enforces positivity and 32-bit range; update the SetSession closure for the TiDBRCPagingSizeBytes entry accordingly (referencing TiDBRCPagingSizeBytes, SetSession, s.RCPagingSizeBytes, tidbOptPositiveInt32, and strconv.Atoi).
🤖 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/session/session.go`:
- Line 3378: Add SQL integration tests that exercise the session variable
tidb_rc_paging_size_bytes end-to-end: write tests that SET and SET GLOBAL the
variable, verify scoping (session vs global), and assert that setting it to 0
disables byte-budget paging in actual queries; mirror the structure and
assertions used in tests/integrationtest/t/sessionctx/setvar.test and cover the
behavior covered by TestBuildCopTasksWithRCPagingSizeBytes so DistSQL requests
pick up RCPagingSizeBytes from the session (the code path that reads
RCPagingSizeBytes in session.go should be exercised by these integration tests).
---
Nitpick comments:
In `@pkg/sessionctx/variable/sysvar.go`:
- Around line 2973-2974: The TiDBRCPagingSizeBytes sysvar SetSession currently
uses TidbOptInt64 with an int(...) cast; replace that with the same pattern used
for MinPagingSize/MaxPagingSize by using tidbOptPositiveInt32 together with
strconv.Atoi to parse/validate the value and assign to s.RCPagingSizeBytes,
ensuring the helper enforces positivity and 32-bit range; update the SetSession
closure for the TiDBRCPagingSizeBytes entry accordingly (referencing
TiDBRCPagingSizeBytes, SetSession, s.RCPagingSizeBytes, tidbOptPositiveInt32,
and strconv.Atoi).
🪄 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: 63954ddf-f0ee-4cee-b1a7-41c3f9ead908
📒 Files selected for processing (8)
pkg/distsql/context/context.gopkg/distsql/request_builder.gopkg/kv/kv.gopkg/session/session.gopkg/sessionctx/vardef/tidb_vars.gopkg/sessionctx/variable/session.gopkg/sessionctx/variable/sysvar.gopkg/store/copr/coprocessor.go
| EnablePaging: vars.EnablePaging, | ||
| MinPagingSize: vars.MinPagingSize, | ||
| MaxPagingSize: vars.MaxPagingSize, | ||
| RCPagingSizeBytes: vars.RCPagingSizeBytes, |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Verify existing test coverage for the new RC paging byte-budget variable/path.
rg -n --glob '*_test.go' 'tidb_rc_paging_size_bytes|RCPagingSizeBytes|paging_size_bytes'
# Check for integration-style SQL test coverage references (if present in repo).
rg -n --glob '*.test' --glob '*.result' 'tidb_rc_paging_size_bytes|paging_size_bytes'Repository: pingcap/tidb
Length of output: 154
🏁 Script executed:
# Inspect the existing test to see what it covers
head -50 pkg/store/copr/coprocessor_test.go | tail -30
sed -n '733,800p' pkg/store/copr/coprocessor_test.go
# Search for RCPagingSizeBytes in session-related test files
rg -l 'RCPagingSizeBytes' pkg/session --glob '*_test.go'
# Check for integration test patterns (testdata, sqllogictests, etc.)
fd -type f -path '*/testdata*' -name '*paging*' -o -path '*/testdata*' -name '*rc*' | head -20
# Broader search for integration/SQL tests mentioning this variable
rg 'tidb_rc_paging' --glob '*.test' --glob '*.sql' --glob '*.result' --glob '*integration*'Repository: pingcap/tidb
Length of output: 3376
🏁 Script executed:
# Find the variable definition for tidb_rc_paging_size_bytes
rg -n 'tidb_rc_paging_size_bytes|RCPagingSizeBytes' pkg/sessionctx/variable pkg/sessionctx/vardef -A 3 -B 1
# Check the session.go GetDistSQLCtx function context
sed -n '3350,3400p' pkg/session/session.go
# Search for existing integration/SQL tests in testdata
fd 'testdata' -type d | head -5
ls -la testdata/ 2>/dev/null | head -20 || echo "No testdata in root"
# Look for other RC (request-scoped) variables to see their test patterns
rg -n 'RCMaxOpenConnections|RCWriteLatencyBase' pkg/session -A 2 -B 1 | head -40Repository: pingcap/tidb
Length of output: 5924
🏁 Script executed:
# List integration test directories
find . -type d -name testdata 2>/dev/null | head -5
# Search for .test files that might cover RC features
find . -name '*.test' -o -name '*.sql' 2>/dev/null | grep -i 'rc\|paging\|resource' | head -10
# Check if session variable tests exist for similar variables
rg -l 'SET tidb_' tests/ testdata/ --glob '*.test' 2>/dev/null | head -5
# Simpler: look for any test file mentioning variable changes
rg 'SET tidb_rc_paging_size_bytes|SET tidb_max_paging' -type f --max-count 5
# Check nearby session variables to understand test patterns
rg 'TiFlashMaxBytesBeforeExternalJoin|TiFlashMaxQueryMemoryPerNode' pkg/session/session.go -B 2 -A 2Repository: pingcap/tidb
Length of output: 1464
🏁 Script executed:
# Check if RC paging tests exist in integration tests
rg 'rc_paging_size_bytes|RCPagingSizeBytes' tests/integrationtest/t/executor/set.test tests/integrationtest/t/sessionctx/setvar.test
# Check what's in setvar.test to understand test pattern
head -50 tests/integrationtest/t/sessionctx/setvar.test
# Search broader for any RC variable testing
rg 'tidb_rc' tests/integrationtest/ --glob '*.test' | head -20Repository: pingcap/tidb
Length of output: 2292
Add SQL integration tests for the new session variable behavior.
Line 3378 wires tidb_rc_paging_size_bytes (a user-facing SESSION/GLOBAL variable) into DistSQL request behavior. While unit test coverage exists at the coprocessor level (TestBuildCopTasksWithRCPagingSizeBytes), SQL integration tests should validate the end-to-end variable behavior, including SET scoping and 0 disabling byte-budget paging (similar to the pattern in tests/integrationtest/t/sessionctx/setvar.test).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/session/session.go` at line 3378, Add SQL integration tests that exercise
the session variable tidb_rc_paging_size_bytes end-to-end: write tests that SET
and SET GLOBAL the variable, verify scoping (session vs global), and assert that
setting it to 0 disables byte-budget paging in actual queries; mirror the
structure and assertions used in tests/integrationtest/t/sessionctx/setvar.test
and cover the behavior covered by TestBuildCopTasksWithRCPagingSizeBytes so
DistSQL requests pick up RCPagingSizeBytes from the session (the code path that
reads RCPagingSizeBytes in session.go should be exercised by these integration
tests).
Signed-off-by: JmPotato <github@ipotato.me>
JmPotato
force-pushed
the
demo/ru-paging-size
branch
from
2dfee7a to
2cc25e2
Compare
|
[FORMAT CHECKER NOTIFICATION] Notice: To remove the 📖 For more info, you can check the "Contribute Code" section in the development guide. |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
DEPS.bzl (1)
8221-8246: Ensure fork references in go.mod are reverted once upstream PRs merge.Both
github.com/tikv/client-go/v2andgithub.com/tikv/pd/clientare currently aliased to JmPotato's forks via go.mod replace directives (lines 373-374) to support the two-phase RU pre-charge/settlement feature. When the corresponding upstream PRs are merged (client-go tikv#1936, PD tikv#10548), remove these replace directives from go.mod. DEPS.bzl will be automatically regenerated on the nextmake bazel_preparerun.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@DEPS.bzl` around lines 8221 - 8246, Remove the temporary go.mod replace directives that point github.com/tikv/client-go/v2 and github.com/tikv/pd/client to the JmPotato forks once the upstream PRs (tikv#1936 and tikv#10548) are merged: delete the replace entries for those module paths in go.mod and then run make bazel_prepare so DEPS.bzl (which currently contains the forked strip_prefix/urls for com_github_jmpotato_client_go_v2 and com_github_jmpotato_pd_client) is regenerated to reference upstream modules.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@DEPS.bzl`:
- Around line 8221-8246: Remove the temporary go.mod replace directives that
point github.com/tikv/client-go/v2 and github.com/tikv/pd/client to the JmPotato
forks once the upstream PRs (tikv#1936 and tikv#10548) are merged: delete the
replace entries for those module paths in go.mod and then run make bazel_prepare
so DEPS.bzl (which currently contains the forked strip_prefix/urls for
com_github_jmpotato_client_go_v2 and com_github_jmpotato_pd_client) is
regenerated to reference upstream modules.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: a5b90f6f-6068-472f-9441-002aaa7e9cbb
📒 Files selected for processing (2)
DEPS.bzlpkg/store/copr/BUILD.bazel
🚧 Files skipped from review as they are similar to previous changes (1)
- pkg/store/copr/BUILD.bazel
|
@JmPotato: 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. |
|
@JmPotato: The following tests 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. |
1 participant
What problem does this PR solve?
Issue Number: ref pingcap/kvproto#1448
Problem Summary:
In concurrent coprocessor workloads under Resource Control, row-count-based paging alone cannot prevent "token cliff" effects where multiple workers exhaust RU budgets simultaneously. This PR introduces byte-budget paging to bound per-page resource consumption by byte size (4 MB), enabling PD's resource controller to pre-charge RU more accurately in Phase 1.
What changed and how does it work?
RC Paging Eligibility (
rcPagingEligible):Paging Injection (
BuildCopIterator):rcPagingSizeBytes = 4MBcopTask→handleTaskOnce→ coprocessor RPC (PagingSizeBytesfield)pagingSizeBytesacross region/lock errorsNew Constants (
paging.go):MinPagingSizeBytes = 256 KB,MaxPagingSizeBytes = 4 MBDependency Updates (
go.mod):JmPotato/kvproto(addspaging_size_bytesproto field)JmPotato/client-go(extractsPagingSizeBytesfromRequestInfo)JmPotato/pd(two-phase RU pre-charge/settlement)Check List
Tests
Side effects
Documentation
Related PRs
Release note
Summary by CodeRabbit
New Features
Tests
Chores