Optimize lock handling in MPP local read#10992
Conversation
ti-chi-bot
Bot
added
release-note-none
|
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:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (12)
🚧 Files skipped from review as they are similar to previous changes (11)
📝 WalkthroughWalkthroughThe change adds lock-aware learner-read exceptions, propagates bypass-lock timestamps through DAG contexts, and enables one local lock-bypass retry before remote fallback. It also adds learner-read tests and advances the ChangesBypass-lock learner retry
Client submodule revision
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant LearnerReadWorker
participant RegionException
participant DAGStorageInterpreter
participant LockResolver
participant DAGContext
LearnerReadWorker->>RegionException: carry lock regions and lock details
RegionException->>DAGStorageInterpreter: report lock_region
DAGStorageInterpreter->>LockResolver: tryGetBypassLock
LockResolver-->>DAGStorageInterpreter: return bypass timestamps
DAGStorageInterpreter->>DAGContext: store bypass timestamps
DAGStorageInterpreter-->>LearnerReadWorker: retry locally or remotely
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 |
ti-chi-bot
Bot
added
size/XL
![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/tests/gtest_learner_read.cpp (1)
44-55: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse
camelCasefor variable names to adhere to coding guidelines.The coding guidelines explicitly state that "Method and variable names should use
camelCase". However, several new variables in these code segments usesnake_case. While the surrounding test file already heavily usessnake_case(e.g.,region_id_200), please consider adopting the mandated convention for newly introduced variables to progressively improve compliance.
dbms/src/Storages/KVStore/tests/gtest_learner_read.cpp#L44-L55: Rename variables such aslock_versionandlock_infotolockVersionandlockInfo.dbms/src/Storages/KVStore/tests/gtest_learner_read.cpp#L196-L256: Rename variables such asregion_id,lock_version,mvcc_query_info,read_index_result,resp, andunavailable_regionsto theircamelCaseequivalents (e.g.,regionId,lockVersion, etc.).🤖 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_learner_read.cpp` around lines 44 - 55, Rename the newly introduced snake_case variables to camelCase throughout dbms/src/Storages/KVStore/tests/gtest_learner_read.cpp: lines 44-55 update lock_version and lock_info, and lines 196-256 update region_id, lock_version, mvcc_query_info, read_index_result, resp, and unavailable_regions, including all references while preserving behavior.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/tests/gtest_learner_read.cpp`:
- Around line 44-55: Rename the newly introduced snake_case variables to
camelCase throughout dbms/src/Storages/KVStore/tests/gtest_learner_read.cpp:
lines 44-55 update lock_version and lock_info, and lines 196-256 update
region_id, lock_version, mvcc_query_info, read_index_result, resp, and
unavailable_regions, including all references while preserving behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: cea8fba9-8e33-474a-b859-23ef7f6db22e
📒 Files selected for processing (1)
dbms/src/Storages/KVStore/tests/gtest_learner_read.cpp
Signed-off-by: xufei <xufeixw@mail.ustc.edu.cn>
Signed-off-by: xufei <xufeixw@mail.ustc.edu.cn>
af90ca7 to
593e1fc
Compare
ti-chi-bot
Bot
added
approved
needs-1-more-lgtm
| try | ||
| { | ||
| auto * cluster = tmt.getKVCluster(); | ||
| if (cluster == nullptr || cluster->lock_resolver == nullptr) |
There was a problem hiding this comment.
| if (cluster == nullptr || cluster->lock_resolver == nullptr) | |
| if unlikely (cluster == nullptr || cluster->lock_resolver == nullptr) |
|
[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
|
/hold |
ti-chi-bot
Bot
added
the
do-not-merge/hold
8b22d94
into
pingcap:feature/release-8.5-materialized-view
JaySon-Huang
left a comment
JaySon-Huang
left a comment
There was a problem hiding this comment.
We can optimize the tiflash shutdown wait timeout when there are many locks to be resolved. tikv/client-c#245 (comment)
Rest LGTM
4 participants
What problem does this PR solve?
Issue Number: N/A
Problem Summary:
Optimize TiFlash MPP/batch cop local learner read lock handling to reduce unnecessary remote reads when local LockCF locks can be handled by a local retry.
What is changed and how it works?
Check List
Tests
Built affected objects locally:
dbms/src/Flash/CMakeFiles/flash_service.dir/Coprocessor/DAGStorageInterpreter.cpp.odbms/src/Storages/KVStore/CMakeFiles/kvstore.dir/Read/LearnerReadWorker.cpp.oRan learner read unit tests locally:
/Users/feixu/dev/pingcap/tiflash/cmake-build-debug/dbms/gtests_dbms --gtest_filter=LearnerReadTest.*Side effects
Documentation
Release note
Summary by CodeRabbit