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
|
[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 |
ti-chi-bot
Bot
added
the
size/XL
📝 WalkthroughWalkthroughThis PR updates Go dependency pins and replaces two tikv client paths with forked versions. It also switches resource-manager mocks to PD metastorage watch responses, refactors session paging checks, and adjusts mockstore PD keyspace lookup behavior. ChangesDependency pin updates
PD metastorage watch API migration
Estimated code review effort: 2 (Simple) | ~12 minutes Possibly related PRs
Suggested labels: Suggested reviewers: 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
Signed-off-by: JmPotato <github@ipotato.me>
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
|
[FORMAT CHECKER NOTIFICATION] Notice: To remove the 📖 For more info, you can check the "Contribute Code" section in the development guide. Notice: To remove the For example:
📖 For more info, you can check the "Contribute Code" section in the development guide. |
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.3233% 75.6693% -0.6541%
================================================
Files 2041 2078 +37
Lines 560726 582364 +21638
================================================
+ Hits 427965 440671 +12706
- Misses 131860 139566 +7706
- Partials 901 2127 +1226
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
|
@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 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.This PR intentionally points TiDB at temporary personal fork commits to validate the cross-repo integration before the upstream PD/client-go dependency pins are available.
What changed and how does it work?
github.com/tikv/pd/clientwithgithub.com/JmPotato/pd/client v0.0.0-20260703063321-7f8ee3eaf371.github.com/tikv/client-go/v2withgithub.com/JmPotato/client-go/v2 v2.0.0-20260703060738-b9da313d68a6for mock compatibility with the updated PD client interface.GetDistSQLCtxpreferResourceGroupsController.GetResourceGroupRuntimeState().HasLimitedBurstwhen deciding whethertidb_paging_size_bytescan be forwarded.make bazel_prepare.Check List
Tests
Manual test steps:
Side effects
Documentation
Release note
Summary by CodeRabbit
Bug Fixes
Chores