Invalidate read-index cache when local lock is found#10986
Conversation
Signed-off-by: xufei <xufeixw@mail.ustc.edu.cn>
ti-chi-bot
Bot
added
release-note
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds region-scoped invalidation through KVStore and read-index workers, clears history-success and MVCC result caches when locks are detected, enhances lock diagnostics, updates a client submodule, and disables one fast add-peer test. ChangesRead-index cache invalidation
Lock diagnostic details
Maintenance updates
Estimated code review effort: 2 (Simple) | ~10 minutes Sequence Diagram(s)sequenceDiagram
participant LearnerReadWorker
participant KVStore
participant ReadIndexWorkerManager
participant ReadIndexWorker
participant ReadIndexDataNode
participant MvccQueryInfo
LearnerReadWorker->>KVStore: invalidateReadIndexCache(region_id)
KVStore->>ReadIndexWorkerManager: invalidateReadIndexCache(region_id)
ReadIndexWorkerManager->>ReadIndexWorker: route region_id
ReadIndexWorker->>ReadIndexDataNode: invalidateReadIndexCache()
ReadIndexDataNode->>ReadIndexDataNode: clear history_success_tasks
LearnerReadWorker->>MvccQueryInfo: invalidateReadIndexResCache(region_id)
LearnerReadWorker->>LearnerReadWorker: mark region unavailable
Possibly related issues
Suggested reviewers: 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 |
|
/hold |
ti-chi-bot
Bot
added
the
do-not-merge/hold
ti-chi-bot
Bot
added
needs-1-more-lgtm
Signed-off-by: xufei <xufeixw@mail.ustc.edu.cn>
Signed-off-by: xufei <xufeixw@mail.ustc.edu.cn>
Signed-off-by: xufei <xufeixw@mail.ustc.edu.cn>
![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/Storages/KVStore/Read/LockException.h (1)
56-64: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winPrefer
fmt::formatfor simple string construction.Using
fmt::formatdirectly is more concise and aligns with the coding guidelines for string construction in C++, avoiding the need for a mutableFmtBuffer.
(Note: If you apply this, you may also be able to remove#include <Common/FmtUtils.h>at line 18 if it is not required for formattinglocked_regions.)♻️ Proposed refactor
- FmtBuffer buffer; - const auto & lock = locks.front(); - buffer.fmtAppend("{}(", lock.first); - if (lock.second) - buffer.append(lock.second->ShortDebugString()); - else - buffer.append("<null>"); - buffer.append(")"); - return buffer.toString(); + const auto & lock = locks.front(); + if (lock.second) + return fmt::format("{}({})", lock.first, lock.second->ShortDebugString()); + return fmt::format("{}(<null>)", lock.first);🤖 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/KVStore/Read/LockException.h` around lines 56 - 64, In the lock string construction within the relevant exception formatting method, replace the mutable FmtBuffer sequence with a direct fmt::format expression that preserves the lock name, ShortDebugString() value, and "<null>" fallback. Remove the Common/FmtUtils.h include if it is no longer needed elsewhere, including for locked_regions formatting.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/KVStore/Read/LockException.h`:
- Around line 56-64: In the lock string construction within the relevant
exception formatting method, replace the mutable FmtBuffer sequence with a
direct fmt::format expression that preserves the lock name, ShortDebugString()
value, and "<null>" fallback. Remove the Common/FmtUtils.h include if it is no
longer needed elsewhere, including for locked_regions formatting.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 9e94e57e-abd1-4523-b60a-524f163ffc3d
📒 Files selected for processing (3)
dbms/src/Storages/KVStore/Read/LockException.hdbms/src/Storages/KVStore/Read/ReadIndexWorker.cppdbms/src/Storages/KVStore/Read/ReadIndexWorker.h
🚧 Files skipped from review as they are similar to previous changes (2)
- dbms/src/Storages/KVStore/Read/ReadIndexWorker.cpp
- dbms/src/Storages/KVStore/Read/ReadIndexWorker.h
|
/test pull-unit-test |
|
/hold cancel |
ti-chi-bot
Bot
removed
the
do-not-merge/hold
|
/test pull-unit-test |
1 similar comment
|
/test pull-unit-test |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: gengliqi, solotzg 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
|
/test pull-unit-test |
2 similar comments
|
/test pull-unit-test |
|
/test pull-unit-test |
Signed-off-by: xufei <xufeixw@mail.ustc.edu.cn>
There was a problem hiding this comment.
🧹 Nitpick comments (1)
dbms/src/Storages/KVStore/tests/gtest_kvstore_fast_add_peer.cpp (1)
859-921: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winKeep the unstable test compilable.
The block comment removes
Cancel5from the translation unit, so future changes can silently break this cancellation path without any compile-time signal. Prefer GoogleTest’sDISABLED_convention (orGTEST_SKIP()with a tracking reference) and retain the test body.Proposed fix
-/* -TEST_F(RegionKVStoreTestFAP, Cancel5) +TEST_F(RegionKVStoreTestFAP, DISABLED_Cancel5) ... -CATCH -*/ +CATCH🤖 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/KVStore/tests/gtest_kvstore_fast_add_peer.cpp` around lines 859 - 921, Restore the RegionKVStoreTestFAP test named Cancel5 to the translation unit and disable it using GoogleTest’s DISABLED_ naming convention, preserving its existing body and cancellation assertions. Do not leave the test inside a block comment; ensure it remains compilable and discoverable as a disabled test.
🤖 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/KVStore/tests/gtest_kvstore_fast_add_peer.cpp`:
- Around line 859-921: Restore the RegionKVStoreTestFAP test named Cancel5 to
the translation unit and disable it using GoogleTest’s DISABLED_ naming
convention, preserving its existing body and cancellation assertions. Do not
leave the test inside a block comment; ensure it remains compilable and
discoverable as a disabled test.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: d3dcb1c1-8c48-4bd2-b781-bd836b875a07
📒 Files selected for processing (1)
dbms/src/Storages/KVStore/tests/gtest_kvstore_fast_add_peer.cpp
|
/test pull-unit-test |
8fe3056
into
pingcap:feature/release-8.5-materialized-view
3 participants
What problem does this PR solve?
Issue Number: ref #10985
Problem Summary:
TiFlash can repeatedly reuse a stale read-index cache after a local LockCF lock is found and the caller resolves the lock on TiKV. The next retry may skip fetching a fresh read index, so it may not wait for the resolve-lock raft log to be applied on the TiFlash learner and can hit the same lock again.
What is changed and how it works?
When
RegionTable::resolveLocksAndWriteRegionstill finds a local lock after wait-index succeeds, TiFlash now invalidates the read-index worker history cache for that region and removes the current query-level read-index cache entry. The next retry will request a fresh read index instead of reusing the stale one.A read-index worker unit test is added to verify that invalidating the cache forces a new read-index request for a smaller read ts that would otherwise use history.
Check List
Tests
Side effects
Documentation
Release note
Summary by CodeRabbit