-
Notifications
You must be signed in to change notification settings - Fork 51
Add TTL mechanism for region cache #225
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
Signed-off-by: gengliqi <gengliqiii@gmail.com>
Signed-off-by: gengliqi <gengliqiii@gmail.com>
📝 WalkthroughWalkthroughAdds TTL-based expiration and cluster_id tracking to the region cache: Region gains an atomic ttl with init/check/update APIs and global TTL knobs; RegionCache uses TTL checks to evict stale regions during locateKey; RPC/PD interfaces and RPCContext gain cluster_id propagation; unit tests and build wiring added. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client
participant RC as RegionCache
participant R as Region
participant Clock as Clock
Client->>RC: locateKey(key)
RC->>R: lookup cached Region for key
RC->>Clock: now = current_time()
RC->>R: checkRegionCacheTTL(now)
alt TTL expired
R-->>RC: false
RC->>RC: evict region (remove cached entry)
RC-->>Client: cache miss → trigger reload
else TTL valid
R-->>RC: true (ttl possibly bumped)
RC-->>Client: return region bounds
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@include/pingcap/kv/RegionCache.h`:
- Line 10: The header RegionCache.h uses std::chrono (e.g., at the code
referencing time_point/durations) but only includes <atomic>, so add `#include`
<chrono> to the header to make it self-contained; update the include block where
<atomic> is present so translation units that include RegionCache.h directly
will compile without requiring incidental chrono includes.
| } | ||
|
|
||
| // now we have ts <= ttl <= ts+regionCacheTTLSec <= newTTL = ts+regionCacheTTLSec+randomJitter | ||
| if (ttl.compare_exchange_weak(current_ttl, new_ttl, std::memory_order_relaxed)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since you don't update current_ttl, it looks to me if ttl.compare_exchange_weak fails, and it will never sucess for the following calls? Of cause by default the loop will break at L55, but if regionCacheTTLJitterSec is 0, it seem will hang in this while loop.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If it fails, current_ttl will be reloaded with the newest value.
Ref: https://en.cppreference.com/w/cpp/atomic/atomic_compare_exchange.html
Atomically compares the object representation(until C++20)value representation(since C++20) of the object pointed to by obj with that of the object pointed to by expected, and if those are bitwise-equal, replaces the former with desired (performs read-modify-write operation). Otherwise, loads the actual value pointed to by obj into *expected (performs load operation).
| #include <kvproto/metapb.pb.h> | ||
| #include <pingcap/kv/RegionCache.h> | ||
|
|
||
| #include "test_helper.h" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
format code
Signed-off-by: gengliqi <gengliqiii@gmail.com>
windtalker
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
|
@windtalker: 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. |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: windtalker 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 |
close #171
close #223
Note that the TTL mechanism is disabled by default, as it may cause performance degradation when TiFlash runs in disaggregated mode. It will be enabled once #224 is resolved.
Summary by CodeRabbit
New Features
Tests
✏️ Tip: You can customize this high-level summary in your review settings.