Optimize MPP lock exception handling - #11019
Merged
ti-chi-bot[bot] merged 2 commits intoAug 11, 2026
Merged
Conversation
Signed-off-by: xufei <xufeixw@mail.ustc.edu.cn>
ti-chi-bot
Bot
added
release-note-none
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughThe change preserves local lock metadata, propagates bypass-lock timestamps through DAG preparation, and adds bounded local retry handling for batch cop reads. Read-index locks remain unavailable regions and use the remote-read path. ChangesLock bypass retry flow
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant LearnerReadWorker
participant RegionException
participant DAGStorageInterpreter
participant LockResolver
participant DAGContext
LearnerReadWorker->>RegionException: provide lock-region metadata
DAGStorageInterpreter->>RegionException: inspect local lock regions
DAGStorageInterpreter->>LockResolver: resolve lock transaction
LockResolver-->>DAGStorageInterpreter: return resolution and bypass timestamp
DAGStorageInterpreter->>DAGContext: store bypass-lock timestamp
DAGStorageInterpreter->>DAGStorageInterpreter: retry locally once or use remote read
Possibly related PRs
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.
🧹 Nitpick comments (2)
dbms/src/Flash/Coprocessor/DAGStorageInterpreter.cpp (1)
146-154: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse
tryLogCurrentExceptionin the catch-all path.The current log discards the exception details. Use
tryLogCurrentExceptionwith the lock-region context before returning the remote-read fallback.As per coding guidelines, “Use
tryLogCurrentException(log, "context")in broadcatch (...)paths.”🤖 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/Coprocessor/DAGStorageInterpreter.cpp` around lines 146 - 154, Update the catch-all path in the bypass-lock check to call tryLogCurrentException with the existing log object and lock-region context before returning false. Replace the generic LOG_WARNING there so the exception details are preserved while retaining the remote-read fallback behavior.Source: Coding guidelines
dbms/src/Flash/Coprocessor/DAGContext.h (1)
285-294: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoffUse camelCase for new C++ variables and parameters.
The new lock-bypass flow introduces snake_case identifiers. Rename these identifiers consistently before extending this API further.
dbms/src/Flash/Coprocessor/DAGContext.h#L285-L294: renamebypass_lock_tsandbypass_lock_ts_.dbms/src/Flash/Coprocessor/TablesRegionsInfo.h#L43-L47: renamebypass_lock_ts.dbms/src/Flash/Coprocessor/TablesRegionsInfo.cpp#L71-L88: renamelocal_region_id_setandbypass_lock_ts.dbms/src/Flash/BatchCoprocessorHandler.cpp#L71-L106: renamebypass_lock_ts.dbms/src/Flash/Disaggregated/WNEstablishDisaggTaskHandler.cpp#L53-L96: renamebypass_lock_ts.dbms/src/Flash/Mpp/MPPTask.cpp#L493-L547: renamebypass_lock_ts.dbms/src/Flash/Coprocessor/DAGStorageInterpreter.cpp#L91-L179: renamewait_bg_resolve_lock_ms,try_get_bypass_lock_max_backoff_ms,read_tso, and related local variables.dbms/src/Storages/KVStore/Read/RegionException.h#L67-L84: renameunavailable_region_,lock_region_,locks_,lock_region, andunavailable_region.dbms/src/Storages/KVStore/Read/LearnerReadWorker.h#L52-L103: renamelock_region_ids,region_id_, andregion_lock_.dbms/src/Storages/KVStore/tests/gtest_learner_read.cpp#L47-L57: renamelock_versionandlock_info.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/Coprocessor/DAGContext.h` around lines 285 - 294, Rename the newly introduced snake_case variables and parameters to camelCase consistently across the listed sites: DAGContext.h lines 285-294; TablesRegionsInfo.h lines 43-47 and TablesRegionsInfo.cpp lines 71-88; BatchCoprocessorHandler.cpp lines 71-106; WNEstablishDisaggTaskHandler.cpp lines 53-96; MPPTask.cpp lines 493-547; DAGStorageInterpreter.cpp lines 91-179; RegionException.h lines 67-84; LearnerReadWorker.h lines 52-103; and gtest_learner_read.cpp lines 47-57. Rename the specified lock-bypass, region, timing, TSO, and test identifiers while updating all references without changing 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/Flash/Coprocessor/DAGContext.h`:
- Around line 285-294: Rename the newly introduced snake_case variables and
parameters to camelCase consistently across the listed sites: DAGContext.h lines
285-294; TablesRegionsInfo.h lines 43-47 and TablesRegionsInfo.cpp lines 71-88;
BatchCoprocessorHandler.cpp lines 71-106; WNEstablishDisaggTaskHandler.cpp lines
53-96; MPPTask.cpp lines 493-547; DAGStorageInterpreter.cpp lines 91-179;
RegionException.h lines 67-84; LearnerReadWorker.h lines 52-103; and
gtest_learner_read.cpp lines 47-57. Rename the specified lock-bypass, region,
timing, TSO, and test identifiers while updating all references without changing
behavior.
In `@dbms/src/Flash/Coprocessor/DAGStorageInterpreter.cpp`:
- Around line 146-154: Update the catch-all path in the bypass-lock check to
call tryLogCurrentException with the existing log object and lock-region context
before returning false. Replace the generic LOG_WARNING there so the exception
details are preserved while retaining the remote-read fallback behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: d0ba1a31-c761-4e91-94a8-7d0678f2435d
📒 Files selected for processing (11)
dbms/src/Flash/BatchCoprocessorHandler.cppdbms/src/Flash/Coprocessor/DAGContext.hdbms/src/Flash/Coprocessor/DAGStorageInterpreter.cppdbms/src/Flash/Coprocessor/TablesRegionsInfo.cppdbms/src/Flash/Coprocessor/TablesRegionsInfo.hdbms/src/Flash/Disaggregated/WNEstablishDisaggTaskHandler.cppdbms/src/Flash/Mpp/MPPTask.cppdbms/src/Storages/KVStore/Read/LearnerReadWorker.cppdbms/src/Storages/KVStore/Read/LearnerReadWorker.hdbms/src/Storages/KVStore/Read/RegionException.hdbms/src/Storages/KVStore/tests/gtest_learner_read.cpp
Signed-off-by: xufei <xufeixw@mail.ustc.edu.cn>
Contributor
Author
|
/test pull-integration-test |
Contributor
Author
|
/test pull-unit-next-gen |
gengliqi
approved these changes
Aug 11, 2026
ti-chi-bot
Bot
added
approved
needs-1-more-lgtm
solotzg
approved these changes
Aug 11, 2026
Contributor
|
[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
Contributor
Contributor
Author
|
/test pull-integration-test |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What problem does this PR solve?
Issue Number: close #11018
Problem Summary:
MPP local read can encounter lock exceptions on many regions for the same snapshot or transaction. Falling back to remote read for each region independently can duplicate resolve-lock work and increase query latency.
What is changed and how it works?
This PR improves the MPP local read lock-exception path:
RegionExceptionso the MPP local read retry path can decide whether local retry is possible.bypass_lock_tswhen a lock can be safely bypassed for the current read timestamp.Check List
Tests
Side effects
Documentation
Release note
Summary by CodeRabbit
Bug Fixes
Tests