LAC: smoothing tokens request and keep in high level (#10997) - #11034
Conversation
close pingcap#10996\n\nSigned-off-by: yongman <yming0221@gmail.com> Signed-off-by: yongman <yming0221@gmail.com>
ti-chi-bot
Bot
added
release-note-none
📝 WalkthroughWalkthroughLocal admission control now uses one-second, capacity-aware token refills. It tracks GAC capacity, limits incremental requests, supports low-token full refills, and adds refill tests. A segment read test now suppresses segment update checks during stable-data merging. ChangesLocal admission token refill
Segment read test stability
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Suggested labels: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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.
Actionable comments posted: 1
🧹 Nitpick comments (1)
dbms/src/Flash/ResourceControl/LocalAdmissionController.h (1)
287-287: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse camelCase for newly added variables.
dbms/src/Flash/ResourceControl/LocalAdmissionController.h#L287-L287: Renamehas_gac_capacitytohasGacCapacity.dbms/src/Flash/ResourceControl/LocalAdmissionController.cpp#L139-L170: Rename new local variables such asrefill_thresholdandhigh_watermarkto camelCase.dbms/src/Flash/ResourceControl/tests/gtest_local_admission_controller.cpp#L22-L113: Rename new test-local variables such asfill_rateandconsumed_tokensto camelCase.As per coding guidelines, “Method and variable names should use
camelCase.”🤖 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.h` at line 287, Rename the newly added variables to camelCase: change has_gac_capacity to hasGacCapacity in dbms/src/Flash/ResourceControl/LocalAdmissionController.h:287-287, rename locals such as refill_threshold and high_watermark in dbms/src/Flash/ResourceControl/LocalAdmissionController.cpp:139-170, and rename test locals such as fill_rate and consumed_tokens in dbms/src/Flash/ResourceControl/tests/gtest_local_admission_controller.cpp:22-113; update all references consistently.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.
Inline comments:
In `@dbms/src/Flash/ResourceControl/tests/gtest_local_admission_controller.cpp`:
- Line 39: Update the shouldRefillToken assertion in the relevant test to pass
std::chrono::milliseconds(500) after start_time instead of
ResourceGroup::REFILL_TOKEN_INTERVAL / 2, ensuring the check uses a non-zero
sub-second duration.
---
Nitpick comments:
In `@dbms/src/Flash/ResourceControl/LocalAdmissionController.h`:
- Line 287: Rename the newly added variables to camelCase: change
has_gac_capacity to hasGacCapacity in
dbms/src/Flash/ResourceControl/LocalAdmissionController.h:287-287, rename locals
such as refill_threshold and high_watermark in
dbms/src/Flash/ResourceControl/LocalAdmissionController.cpp:139-170, and rename
test locals such as fill_rate and consumed_tokens in
dbms/src/Flash/ResourceControl/tests/gtest_local_admission_controller.cpp:22-113;
update all references consistently.
🪄 Autofix
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 Plus
Run ID: 4550d572-01de-4973-aa9e-8420726ea7e8
📒 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
| group.smooth_ru_consumption_speed = 0; | ||
| group.consumeResource(consumed_tokens, 0); | ||
|
|
||
| EXPECT_FALSE(group.shouldRefillToken(start_time + ResourceGroup::REFILL_TOKEN_INTERVAL / 2)); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🌐 Web query:
According to the C++ standard library, what value does std::chrono::seconds{1} / 2 produce?
💡 Result:
In the C++ standard library, the expression std::chrono::seconds{1} / 2 produces a std::chrono::seconds object with a count of 0 [1][2][3]. When a std::chrono::duration (such as std::chrono::seconds) is divided by a scalar value, the division is performed on the internal tick count of the duration [1][4]. The result of this operation is a new std::chrono::duration object of the same type [1][2][3]. In this case: 1. std::chrono::seconds{1} has an internal tick count of 1. 2. The division operator performs integer division: 1 / 2, which equals 0. 3. The resulting object is std::chrono::seconds{0}.
Citations:
- 1: https://cppreference.dev/w/cpp/chrono/duration/operator_arith4
- 2: https://timsong-cpp.github.io/cppwp/std23/time.duration
- 3: https://timsong-cpp.github.io/cppwp/n4868/time.duration
- 4: https://cplusplus.com/reference/chrono/duration/operators/
🏁 Script executed:
#!/bin/bash
set -e
file="dbms/src/Flash/ResourceControl/tests/gtest_local_admission_controller.cpp"
rg -n -C 8 "REFILL_TOKEN_INTERVAL|shouldRefillToken|start_time" "$file"
rg -n -C 4 "REFILL_TOKEN_INTERVAL" dbms/src/Flash/ResourceControl
python3 - <<'PY'
from datetime import timedelta
# Model the relevant duration arithmetic: std::chrono::seconds has an
# integral tick representation, so division by an integer truncates.
seconds_count = 1
print("seconds{1} / 2 count:", seconds_count // 2)
print("500ms count in seconds:", timedelta(milliseconds=500).total_seconds())
PYRepository: pingcap/tiflash
Length of output: 10265
Use a non-zero sub-second duration.
ResourceGroup::REFILL_TOKEN_INTERVAL / 2 evaluates to zero seconds. The assertion therefore checks start_time, not a time before the refill interval. Use std::chrono::milliseconds(500) instead.
🤖 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/tests/gtest_local_admission_controller.cpp` at
line 39, Update the shouldRefillToken assertion in the relevant test to pass
std::chrono::milliseconds(500) after start_time instead of
ResourceGroup::REFILL_TOKEN_INTERVAL / 2, ensuring the check uses a non-zero
sub-second duration.
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: JaySon-Huang 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 |
[LGTM Timeline notifier]Timeline:
|
|
/test pull-unit-test |
…te+mergeDeltaAll (pingcap#10898) close pingcap#10897\n\nSigned-off-by: JaySon-Huang <tshent@qq.com>
There was a problem hiding this comment.
🧹 Nitpick comments (1)
dbms/src/Storages/DeltaMerge/tests/gtest_segment_read_task.cpp (1)
656-657: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRename
fp_guardto follow the C++ naming rule.Line 656 introduces a local variable with snake_case. Rename it to
fpGuard. Keep the scope guard in this block.As per coding guidelines, “Method and variable names should use
camelCase.”Proposed fix
- auto fp_guard + auto fpGuard🤖 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/Storages/DeltaMerge/tests/gtest_segment_read_task.cpp` around lines 656 - 657, Rename the local scope-guard variable in the test block from fp_guard to fpGuard, while preserving the existing ext::make_scope_guard behavior and scope.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 `@dbms/src/Storages/DeltaMerge/tests/gtest_segment_read_task.cpp`:
- Around line 656-657: Rename the local scope-guard variable in the test block
from fp_guard to fpGuard, while preserving the existing ext::make_scope_guard
behavior and scope.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 7634d53d-2fd5-4c52-a3f0-9f15c61fe961
📒 Files selected for processing (1)
dbms/src/Storages/DeltaMerge/tests/gtest_segment_read_task.cpp
|
/retest |
3ca432f
into
pingcap:release-8.5-20260811-v8.5.7
3 participants
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:
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
Bug Fixes
Tests