*: clarify minimum max paging size semantics#67469
Conversation
ti-chi-bot
Bot
added
do-not-merge/needs-linked-issue
release-note-none
|
@D3Hunter 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
Bot
added
the
size/M
|
Hi @D3Hunter. 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. |
📝 WalkthroughWalkthroughThe PR updates paging size constraint constants across the codebase, replacing Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes 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)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.11.4)Command failed 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
lgtm
and removed
needs-1-more-lgtm
[LGTM Timeline notifier]Timeline:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (3)
pkg/util/paging/paging.go (1)
58-58: Avoid hard-coding the seek-count base (8).Line 58 hard-codes
8, which is implicitlymaxPagingSizeShift + 1. Deriving it from constants keeps this stable if paging growth config changes later.♻️ Proposed refactor
- return float64(8 + (expectCnt-pagingGrowingSum+MinAllowedMaxPagingSize-1)/MinAllowedMaxPagingSize) + return float64(uint64(maxPagingSizeShift+1) + (expectCnt-pagingGrowingSum+MinAllowedMaxPagingSize-1)/MinAllowedMaxPagingSize)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/util/paging/paging.go` at line 58, The return expression currently hard-codes the seek-count base as 8; replace that literal with a derived value using the existing constant (maxPagingSizeShift + 1) so the computation becomes stable when paging growth config changes—locate the return in pkg/util/paging/paging.go (the expression returning float64(8 + (expectCnt-pagingGrowingSum+MinAllowedMaxPagingSize-1)/MinAllowedMaxPagingSize)) and substitute 8 with (maxPagingSizeShift + 1), keeping the rest of the formula intact and preserving types (cast to float64 as needed).pkg/store/copr/coprocessor.go (2)
1768-1771: Polish the next-gen paging comment for clarity.The intent is correct; just a small grammar/clarity cleanup would make this easier to scan.
✏️ Suggested comment wording
-// For next-gen, the storage may return paging range even if paging is not -// enabled. coprocessor have a max_resp_size to control the response size, -// the default is 32MiB +// For next-gen requests, storage may return a paging range even when paging +// is not explicitly enabled. Coprocessor response size is controlled by +// max_resp_size (default: 32 MiB).🤖 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 1768 - 1771, Polish the comment above the paging check in coprocessor: clarify that next-gen storage can return a paging range even if paging isn't enabled, mention that the coprocessor enforces a response size limit via max_resp_size (default 32MiB), and then leave the conditional that checks worker.req.Paging.Enable || copResp.GetRange() unchanged; replace the existing comment with a concise, grammatically-correct sentence referencing the coprocessor, next-gen storage, paging range, and max_resp_size so readers immediately understand why the check exists.
93-96: Improve wording for the memory warning comment.Good intent, but the sentence is awkward (“this might be a large memory”). Consider tightening for readability.
✏️ Suggested comment wording
-// the returned kv.Response might hold at most concurrency * resp-channel-size -// number of coprocessor responses depending on the request type, this might be -// a large memory. +// the returned kv.Response might hold up to +// concurrency * resp-channel-size coprocessor responses (depending on request type), +// which can consume a large amount of memory.🤖 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 93 - 96, The existing comment about memory usage is awkward; update the comment that references kv.Response and the resp-channel-size (used by BuildCopIterator and buildCopTasks) to a clearer phrasing such as: "The returned kv.Response may contain up to concurrency * resp-channel-size coprocessor responses, which can consume a large amount of memory depending on request type and channel sizes." Locate the comment near BuildCopIterator/buildCopTasks and replace the sentence "this might be a large memory" with this clearer wording.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@pkg/store/copr/coprocessor.go`:
- Around line 1768-1771: Polish the comment above the paging check in
coprocessor: clarify that next-gen storage can return a paging range even if
paging isn't enabled, mention that the coprocessor enforces a response size
limit via max_resp_size (default 32MiB), and then leave the conditional that
checks worker.req.Paging.Enable || copResp.GetRange() unchanged; replace the
existing comment with a concise, grammatically-correct sentence referencing the
coprocessor, next-gen storage, paging range, and max_resp_size so readers
immediately understand why the check exists.
- Around line 93-96: The existing comment about memory usage is awkward; update
the comment that references kv.Response and the resp-channel-size (used by
BuildCopIterator and buildCopTasks) to a clearer phrasing such as: "The returned
kv.Response may contain up to concurrency * resp-channel-size coprocessor
responses, which can consume a large amount of memory depending on request type
and channel sizes." Locate the comment near BuildCopIterator/buildCopTasks and
replace the sentence "this might be a large memory" with this clearer wording.
In `@pkg/util/paging/paging.go`:
- Line 58: The return expression currently hard-codes the seek-count base as 8;
replace that literal with a derived value using the existing constant
(maxPagingSizeShift + 1) so the computation becomes stable when paging growth
config changes—locate the return in pkg/util/paging/paging.go (the expression
returning float64(8 +
(expectCnt-pagingGrowingSum+MinAllowedMaxPagingSize-1)/MinAllowedMaxPagingSize))
and substitute 8 with (maxPagingSizeShift + 1), keeping the rest of the formula
intact and preserving types (cast to float64 as needed).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 17f864d0-a963-403c-b5cb-4576e55a05f7
📒 Files selected for processing (6)
pkg/distsql/request_builder_test.gopkg/kv/kv.gopkg/sessionctx/vardef/tidb_vars.gopkg/store/copr/coprocessor.gopkg/util/paging/paging.gopkg/util/paging/paging_test.go
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #67469 +/- ##
================================================
- Coverage 77.8038% 77.5359% -0.2679%
================================================
Files 2023 1943 -80
Lines 556529 544724 -11805
================================================
- Hits 433001 422357 -10644
- Misses 121782 122359 +577
+ Partials 1746 8 -1738
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
|
🔍 Starting code review for this PR... |
|
🔍 New commits detected — starting re-review... |
1 similar comment
|
🔍 New commits detected — starting re-review... |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: joechenrh, terry1purcell, windtalker 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 |
|
/retest-required |
|
@terry1purcell: PRs from untrusted users cannot be marked as trusted with 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 |
6 participants
What problem does this PR solve?
Issue Number: ref #61702
Problem Summary:
For next-gen coprocessor requests, storage can still return paging ranges even when paging is not explicitly enabled. The previous naming around
MaxPagingSizedid not clearly reflect its role as a lower bound for safe paging growth behavior.What changed and how does it work?
MaxPagingSizetoMinAllowedMaxPagingSizeto clarify semantics.GrowPagingSizeby enforcingmax(maxv, MinAllowedMaxPagingSize).tidb_max_paging_sizeand request-builder test expectations with this lower bound.Check List
Tests
Side effects
Documentation
Release note
Please refer to Release Notes Language Style Guide to write a quality release note.
Summary by CodeRabbit
Tests
Documentation