session: use resource group runtime state for paging#69633
Conversation
ti-chi-bot
Bot
added
do-not-merge/needs-linked-issue
do-not-merge/needs-tests-checked
do-not-merge/release-note-label-needed
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthrough
ChangesResource group paging eligibility
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
ti-chi-bot
Bot
added
release-note
JmPotato
force-pushed
the
codex/rc-effective-hard-limit
branch
from
a71b708 to
23f168d
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
pkg/session/session.go (1)
3552-3563: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd unit test coverage for the new paging helper.
resourceGroupAllowsPagingSizeBytesis a pure function with three distinct branches (nil/empty guard, runtime-state path, infoschema fallback) implementing the exact behavior this PR is meant to fix. A focused unit test exercising each branch would guard against regressions in this hard-limit detection logic.🤖 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 `@pkg/session/session.go` around lines 3552 - 3563, Add focused unit tests for resourceGroupAllowsPagingSizeBytes to cover all three branches: the nil/empty guard, the ResourceGroupsController runtime-state path, and the InfoSchema fallback. Use the function name resourceGroupAllowsPagingSizeBytes and the Domain/ResourceGroupsController/GetResourceGroupRuntimeState/InfoSchema.ResourceGroupByName paths to build table-driven cases that assert the expected boolean result for each scenario.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@pkg/session/session.go`:
- Around line 3552-3563: Add focused unit tests for
resourceGroupAllowsPagingSizeBytes to cover all three branches: the nil/empty
guard, the ResourceGroupsController runtime-state path, and the InfoSchema
fallback. Use the function name resourceGroupAllowsPagingSizeBytes and the
Domain/ResourceGroupsController/GetResourceGroupRuntimeState/InfoSchema.ResourceGroupByName
paths to build table-driven cases that assert the expected boolean result for
each scenario.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 45c5e12d-58be-4f33-b7e6-c4df9a4a82d0
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (7)
DEPS.bzlgo.modpkg/domain/infosync/BUILD.bazelpkg/domain/infosync/resource_manager_client.gopkg/executor/adapter_internal_test.gopkg/session/session.gopkg/store/mockstore/unistore/pd.go
There was a problem hiding this comment.
🧹 Nitpick comments (1)
pkg/session/session.go (1)
3552-3563: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a comment explaining the hard-limit/paging rationale.
The logic here is non-obvious: paging is only allowed when the resource group has a limited (hard-capped) burst —
state.HasLimitedBurstorGetBurstLimitAdjusted() >= 0— and disabled when the group is unlimited. Without context, a reader could easily assume the opposite (that an unlimited group would allow larger paging). A short comment on why hard-capped burst is required to safely enable paging (and why runtime state is preferred over InfoSchema) would aid future maintainers.📝 Proposed comment addition
+// resourceGroupAllowsPagingSizeBytes reports whether paging can be enabled for the given +// resource group. Paging is only safe when the group has a hard-capped (limited) burst, +// since an unlimited-burst group may not enforce the same throttling contract that paging +// relies on. Runtime resource-group state is preferred; InfoSchema's static burst-limit +// metadata is used as a fallback until runtime state is available (e.g. right after group +// creation or a service-limit override). func resourceGroupAllowsPagingSizeBytes(dom *domain.Domain, resourceGroupName string) bool {Based on coding guidelines: "Comments SHOULD explain non-obvious intent, constraints, invariants, concurrency guarantees, SQL/compatibility contracts, or important performance trade-offs."
🤖 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 `@pkg/session/session.go` around lines 3552 - 3563, Add a short explanatory comment in resourceGroupAllowsPagingSizeBytes clarifying that paging is only enabled for resource groups with a hard-limited burst (state.HasLimitedBurst or GetBurstLimitAdjusted() >= 0), not for unlimited groups. Mention that the runtime state from ResourceGroupsController is preferred when available because it reflects the current effective limit, with InfoSchema used only as a fallback.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.
Nitpick comments:
In `@pkg/session/session.go`:
- Around line 3552-3563: Add a short explanatory comment in
resourceGroupAllowsPagingSizeBytes clarifying that paging is only enabled for
resource groups with a hard-limited burst (state.HasLimitedBurst or
GetBurstLimitAdjusted() >= 0), not for unlimited groups. Mention that the
runtime state from ResourceGroupsController is preferred when available because
it reflects the current effective limit, with InfoSchema used only as a
fallback.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 5ea9a75c-7b3f-40a0-be55-9d52fa63001f
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (7)
DEPS.bzlgo.modpkg/domain/infosync/BUILD.bazelpkg/domain/infosync/resource_manager_client.gopkg/executor/adapter_internal_test.gopkg/session/session.gopkg/store/mockstore/unistore/pd.go
🚧 Files skipped from review as they are similar to previous changes (6)
- pkg/domain/infosync/BUILD.bazel
- pkg/executor/adapter_internal_test.go
- pkg/domain/infosync/resource_manager_client.go
- go.mod
- pkg/store/mockstore/unistore/pd.go
- DEPS.bzl
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #69633 +/- ##
================================================
- Coverage 76.3186% 74.0014% -2.3172%
================================================
Files 2041 2056 +15
Lines 560073 578302 +18229
================================================
+ Hits 427440 427952 +512
- Misses 131732 150027 +18295
+ Partials 901 323 -578
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
JmPotato
force-pushed
the
codex/rc-effective-hard-limit
branch
from
23f168d to
b9c93dd
Compare
JmPotato
force-pushed
the
codex/rc-effective-hard-limit
branch
2 times, most recently
from
1a72eaa to
a6a551e
Compare
|
/retest |
JmPotato
force-pushed
the
codex/rc-effective-hard-limit
branch
from
a6a551e to
f60f338
Compare
ti-chi-bot
Bot
added
size/S
|
@YuhaoZhang00: adding LGTM is restricted to approvers and reviewers in OWNERS files. DetailsIn response to this: 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. |
|
/retest |
ti-chi-bot
Bot
added
approved
needs-1-more-lgtm
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: cfzjywxk, rleungx, YuhaoZhang00 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 |
ti-chi-bot
Bot
added
lgtm
and removed
needs-1-more-lgtm
|
/test check-dev |
Signed-off-by: JmPotato <github@ipotato.me>
JmPotato
force-pushed
the
codex/rc-effective-hard-limit
branch
from
f60f338 to
2050776
Compare
|
/test check-dev2 |
ref pingcap#68085 (cherry picked from commit 10292a4) Signed-off-by: JmPotato <github@ipotato.me>
ref pingcap#68085 (cherry picked from commit 10292a4) Signed-off-by: JmPotato <github@ipotato.me>
ref pingcap#68085 (cherry picked from commit 10292a4) Signed-off-by: JmPotato <github@ipotato.me>
ref pingcap#68085 (cherry picked from commit 10292a4) Signed-off-by: JmPotato <github@ipotato.me>
4 participants
What problem does this PR solve?
Issue Number: ref #68085, tikv/pd#10612
Problem Summary:
PD resource control now exposes whether the runtime request-unit token bucket has a limited burst. TiDB's
tidb_paging_size_bytesgate still depended only on the static resource-group metadata in InfoSchema, so it could miss limited-burst settings produced by service-limit override logic.The required PD/client-go versions and TiDB mock compatibility are already present on master through #67941, so this PR now only changes the session-side paging gate.
What changed and how does it work?
GetDistSQLCtxpreferResourceGroupsController.GetResourceGroupRuntimeState().HasLimitedBurstwhen deciding whethertidb_paging_size_bytescan be forwarded.Check List
Tests
Unit test and static checks:
Real TiKV E2E verification (temporary local regression test):
tiup playground v8.5.4 --mode tikv-slimcluster with a locally built PD and a real TiKV store.100. InfoSchema still reported thedefaultresource group asBURSTABLE = UNLIMITED, while the runtime state converged toHasLimitedBurst = true.tidb_paging_size_bytes = 4194304and captured the actual outboundcoprocessor.RequestatonBeforeSendReqCtx../tools/check/failpoint-go-test.sh tests/realtikvtest/sessiontest -run '^TestPagingSizeBytesUsesRuntimeLimitedBurst$' -count=1PagingSizeBytesupstream/master(52f7a7a3e6)HasLimitedBurst = true04194304)f60f3384cd)HasLimitedBurst = true4194304This verifies the complete path from PD's runtime service-limit override through TiDB's resource-group runtime state to the outbound Cop RPC. It does not verify TiKV server-side page truncation/resume behavior because the stock TiKV v8.5.4 binary does not implement this request field.
Side effects
Documentation
Release note
Summary by CodeRabbit