LAC: smoothing tokens request and keep in high level - #10997
Conversation
Signed-off-by: yongman <yming0221@gmail.com>
ti-chi-bot
Bot
added
release-note-none
📝 WalkthroughWalkthrough
ChangesToken Refill Control
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant LocalAdmissionController
participant ResourceGroup
participant TokenBucket
LocalAdmissionController->>ResourceGroup: shouldRefillToken(current_tick)
ResourceGroup->>TokenBucket: getCapacity()
TokenBucket-->>ResourceGroup: configured capacity
ResourceGroup-->>LocalAdmissionController: refill decision
LocalAdmissionController->>ResourceGroup: buildRequestInfoIfNecessary()
ResourceGroup-->>LocalAdmissionController: acquire_tokens
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
dbms/src/Flash/ResourceControl/LocalAdmissionController.cpp (1)
505-506: 🚀 Performance & Scalability | 🔵 TrivialRefill trigger increases GAC request cadence; confirm GAC-side load headroom.
With
shouldRefillTokengating on an 80% high-watermark and a 1sREFILL_TOKEN_INTERVAL, every normal-mode group whose bucket sits below 80% will now emit a token request eachmainLooptick (~1s), versus the prior low-token-only fetch. Under steady consumption groups tend to stay below the watermark, so per-group GAC request frequency rises materially with the number of active resource groups. Worth confirming GAC can absorb the aggregate rate at your expected group count, and consider a metric/alert ontype_request_gac_countto watch for request storms.🤖 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 `@dbms/src/Flash/ResourceControl/LocalAdmissionController.cpp` around lines 505 - 506, Review the refill condition in the local admission-control main loop, specifically the shouldRefillToken call combined with local_keyspace_low_token_resource_groups. Confirm the resulting per-group GAC request cadence is within expected capacity at the maximum active resource-group count, and add monitoring or an alert for type_request_gac_count to detect request storms.
🤖 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 `@dbms/src/Flash/ResourceControl/LocalAdmissionController.cpp`:
- Around line 505-506: Review the refill condition in the local
admission-control main loop, specifically the shouldRefillToken call combined
with local_keyspace_low_token_resource_groups. Confirm the resulting per-group
GAC request cadence is within expected capacity at the maximum active
resource-group count, and add monitoring or an alert for type_request_gac_count
to detect request storms.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 32727b95-3f40-472e-8dad-bd7f3ac23edd
📒 Files selected for processing (4)
dbms/src/Flash/ResourceControl/LocalAdmissionController.cppdbms/src/Flash/ResourceControl/LocalAdmissionController.hdbms/src/Flash/ResourceControl/TokenBucket.hdbms/src/Flash/ResourceControl/tests/gtest_local_admission_controller.cpp
ti-chi-bot
Bot
added
needs-1-more-lgtm
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: JaySon-Huang, Lloyd-Pottiger 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
[LGTM Timeline notifier]Timeline:
|
|
/cherry-pick release-nextgen-202603 |
|
@yongman: new pull request created to branch 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 ti-community-infra/tichi repository. |
|
@yongman: 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. |
|
/cherry-pick release-8.5 |
|
@JaySon-Huang: new pull request created to branch 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 ti-community-infra/tichi repository. |
This is a cherry-pick of #10997 ### What problem does this PR solve? Issue Number: close #10996 ## Summary This change improves TiFlash Local Admission Controller token refill behavior to keep the local token bucket near a high watermark without requesting a large amount of tokens in a single GAC request. ## Problem The previous acquire calculation was based only on predicted consumption: ```text acquire_tokens = max(smoothed_speed * 5s * 1.1 - remaining_tokens, 0) ``` When the smoothed consumption speed was underestimated, a small positive token balance could make `acquire_tokens` zero. The local balance would then remain low and could be exhausted by a traffic burst, causing unexpected throttling. Always refilling directly to the full bucket capacity would avoid this problem, but could transfer and retain too many tokens in TiFlash at once, reducing the tokens available to other clients such as TiDB. ## Changes - Added a proactive refill watermark at 80% of the local high watermark. - Added a one-second refill check interval in normal mode. - Included proactive refill checks in addition to the existing low-token and consumption-report triggers. - Added incremental token acquisition for normal refills: ```text deficit = high_watermark - remaining_tokens fallback_batch = min( 5000, high_watermark * 20% ) incremental_batch = max( smoothed_consumption_speed * 1s * 1.1, fallback_batch ) acquire_tokens = min(deficit, incremental_batch) ``` - Preserved emergency refill behavior when the bucket reaches the existing low-token threshold. In that case, the incremental limit is bypassed to avoid request throttling. - Before the first GAC token response, the Resource Group `fill_rate` is used as the local high watermark. - After the first GAC response, the capacity assigned by GAC to the current client is used as the high watermark. - Added `has_gac_capacity` state to distinguish the global Resource Group burst limit from the capacity assigned to the local client. - Added a read-only `TokenBucket::getCapacity()` accessor. - Kept the low-token threshold based on the actual post-grant token balance. This prevents a capacity increase from immediately classifying the bucket as low-token and triggering a large emergency refill. - Preserved the existing five-second consumption reporting period and GAC target request period. - Did not change RU accounting, token deduction, GAC grant handling, or trickle-mode semantics. ## Resulting Behavior - TiFlash starts refilling before the local bucket reaches a critically low balance. - Normal refill requests are spread across smaller requests instead of immediately filling the entire capacity. - High-throughput workloads can still request approximately one second of predicted consumption per refill. - Low-token conditions retain an emergency path that prioritizes avoiding unexpected query throttling. - A newly started TiFlash instance does not use the global Resource Group burst limit as its initial local refill target. - Unused tokens are less likely to be transferred from GAC to TiFlash in one large request, reducing the impact on other clients sharing the Resource Group. ##Test During bench tpch workload, after acquire tokens from GAC, the `remaining_tokens` keeps close to the high watermark. <img width="1830" height="609" alt="image" src="https://github.com/user-attachments/assets/53effdcc-7392-4967-b052-e418fc8d7927" /> ### Check List Tests <!-- At least one of them must be included. --> - [ ] Unit test - [ ] Integration test - [x] Manual test (add detailed scripts or steps below) - [ ] No code Side effects - [ ] Performance regression: Consumes more CPU - [ ] 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 <!-- bugfix or new feature needs a release note --> ```release-note None ``` <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Bug Fixes** * Improved resource token refilling for more consistent capacity availability. * Added capacity-aware refill behavior, including incremental requests and full refills when resources run low. * Improved handling of capacity supplied through admission control responses. * Prevented background maintenance activity from interfering with stable data operations. * **Tests** * Added coverage for startup refills, incremental replenishment, predicted consumption, and low-capacity recovery. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Signed-off-by: yongman <yming0221@gmail.com> Co-authored-by: JaySon <tshent@qq.com>
4 participants
What problem does this PR solve?
Issue Number: close #10996
Summary
This change improves TiFlash Local Admission Controller token refill behavior to keep the local token bucket near a high watermark without requesting a large amount of tokens in a single GAC request.
Problem
The previous acquire calculation was based only on predicted consumption:
When the smoothed consumption speed was underestimated, a small positive token balance could make
acquire_tokenszero. The local balance would then remain low and could be exhausted by a traffic burst, causing unexpected throttling.Always refilling directly to the full bucket capacity would avoid this problem, but could transfer and retain too many tokens in TiFlash at once, reducing the tokens available to other clients such as TiDB.
Changes
fill_rateis used as the local high watermark.has_gac_capacitystate to distinguish the global Resource Group burst limit from the capacity assigned to the local client.TokenBucket::getCapacity()accessor.Resulting Behavior
##Test
During bench tpch workload, after acquire tokens from GAC, the
remaining_tokenskeeps close to the high watermark.Check List
Tests
Side effects
Documentation
Release note
Summary by CodeRabbit