[fix](fe) Guard auto partition result against concurrent drops#65282
Open
zclllyybb wants to merge 1 commit into
Open
[fix](fe) Guard auto partition result against concurrent drops#65282zclllyybb wants to merge 1 commit into
zclllyybb wants to merge 1 commit into
Conversation
Contributor
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
Contributor
Author
|
/review |
Contributor
Author
|
run buildall |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Automated review completed. I did not find a PR-scoped issue that needs an inline comment.
Critical checkpoint conclusions:
- Goal/test: the patch snapshots the auto-partition result metadata under the table read lock and returns a normal error when a just-created partition or partition item has already disappeared. Existing FE tests cover successful createPartition paths, and the PR reports
./build.sh --feplusgit diff --check; I did not rerun the FE build in this runner becausethirdparty/installed/bin/protocis missing. - Scope: the change is limited to
FrontendServiceImpl.createPartitionand stays focused on the concurrent retention-drop result-builder race. - Concurrency/locks: table metadata reads for partition count, partition item, tablet IDs, mutability, random-distribution state, and quorum now happen under the table read lock. Backend/cloud tablet-location and cache work remains outside that lock, so I did not find a metadata-lock RPC or external-IO issue.
- Parallel paths: I inspected
replacePartitionas the closest parallel tablet-building path. It is an unchanged insert-overwrite path with its own task lock and comments allowing outside-transaction changes to fail naturally, so I did not find a distinct PR-scoped issue there. - Compatibility/persistence/config: no thrift schema, persisted metadata, storage format, or config item changes were introduced.
- Tests: I did not see a new automated test for the concurrent-drop branch; the current test tree exercises successful auto-partition creation. Given the narrow guarded snapshot change and static review constraints, I am not blocking on this.
- Observability/performance: the new retry errors are logged with partition context, and the heavier BE/cloud resolution remains outside the table lock.
Subagent conclusions:
optimizer-rewrite: no optimizer/rewrite candidate in the initial pass; convergence returnedNO_NEW_VALUABLE_FINDINGS.tests-session-config: no tests/session/config candidate in the initial pass; convergence returnedNO_NEW_VALUABLE_FINDINGS.- Convergence round 1 completed with all live subagents reporting
NO_NEW_VALUABLE_FINDINGSfor the same ledger and empty inline comment set.
User focus: no additional user-provided focus was supplied.
Validation performed: read the review prompt, repository code-review skill, required AGENTS files, existing review context, changed-file list, and shared ledger; reviewed the GitHub patch and surrounding createPartition/replacePartition code paths; verified the live PR head is still 489c29166f6f0af3be744a5667ef319bae1f6a2b; confirmed there are no existing inline PR comments; ran git diff --check -- fe/fe-core/src/main/java/org/apache/doris/service/FrontendServiceImpl.java.
zclllyybb
force-pushed
the
codex-fix-doris-26817
branch
2 times, most recently
from
5327c7d to
5f01f5b
Compare
Contributor
Author
|
run buildall |
Contributor
Author
|
/review |
Contributor
There was a problem hiding this comment.
Review result: request changes.
I found one correctness issue: the new snapshot pattern fixes createPartition, but the parallel replacePartition RPC still builds result metadata after releasing taskLock and can race task-group rollback of temp partitions into an FE NPE instead of returning a normal error status.
Checkpoint conclusions: goal is clear and mostly focused; concurrency/lifecycle is the main risk; no new config, persistence, or protocol format change; the functionally parallel code path is incomplete; tests were not runnable locally because this checkout is missing thirdparty/installed and thirdparty/installed/bin/protoc. Static validation: git diff --check -- fe/fe-core/src/main/java/org/apache/doris/service/FrontendServiceImpl.java passed.
User focus: no additional user-provided focus.
Subagent conclusions: optimizer-rewrite reported NO_NEW_VALUABLE_FINDINGS. tests-session-config proposed TSC-1; main verification accepted it as MAIN-1 with narrowed evidence around replacePartition rollback rather than the broader retention wording. Convergence round 1 ended with both subagents reporting NO_NEW_VALUABLE_FINDINGS for the current ledger/comment set.
| List<PartitionResultSnapshot> partitionSnapshots = new ArrayList<>(); | ||
|
|
||
| olapTable.readLock(); | ||
| try { |
Contributor
There was a problem hiding this comment.
Can we apply this same snapshot pattern to replacePartition before returning? The BE auto-detect overwrite path calls FrontendService.replacePartition with the same routing fields (be_endpoint, load_to_single_tablet, enable_adaptive_random_bucket, query_id) from VRowDistribution::_replace_overwriting_partition. In replacePartition, after taskLock is released, the response builder still does olapTable.getPartition(partitionId) and immediately dereferences partition.getId() / partitionInfo.getItem(...) without a table read lock or null check. InsertOverwriteManager.taskGroupFail can acquire the task lock after this point and roll back the temp partitions via InsertOverwriteUtil.dropPartitions, so a failed or cancelled auto-detect overwrite can still race this response builder into an FE NPE instead of a normal error status. Please snapshot the replace result metadata under the table read lock, or otherwise keep the task/table protection until the metadata needed for the response has been captured, and return a retry/failure status if a result partition or item has already disappeared.
Contributor
TPC-H: Total hot run time: 29529 ms |
### What problem does this PR solve?
Issue Number: None
Related PR: None
Problem Summary: Auto partition creation can race with dynamic partition retention after new partitions are added, and auto-detect insert-overwrite replacePartition can race with task-group rollback after taskLock is released. Both paths build RPC result partition metadata by reading table partitions and partition info; if a result partition or partition item disappears in that window, FE can hit a null pointer while constructing partition and tablet metadata. This change snapshots the required partition metadata through a shared helper under the table read lock. replacePartition captures that snapshot before releasing the task lock, while backend, cloud tablet-location, and cache work still run outside metadata locks. If a result partition has already disappeared before the snapshot, the RPC now returns a normal retry/failure status instead of throwing an FE null pointer.
### Release note
None
### Check List (For Author)
- Test: Manual test
- mvn checkstyle:check -rf :fe-core
- ./build.sh --fe
- git diff --check -- fe/fe-core/src/main/java/org/apache/doris/service/FrontendServiceImpl.java
- Behavior changed: Yes, concurrent result-partition drops now return a retry/failure error instead of throwing an FE null pointer.
- Does this need documentation: No
Contributor
TPC-DS: Total hot run time: 179564 ms |
zclllyybb
force-pushed
the
codex-fix-doris-26817
branch
from
5f01f5b to
cdffecb
Compare
Contributor
Author
|
run buildall |
1 similar comment
Contributor
Author
|
run buildall |
Contributor
TPC-H: Total hot run time: 29208 ms |
Contributor
TPC-DS: Total hot run time: 180916 ms |
Contributor
ClickBench: Total hot run time: 25.04 s |
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: None
Related PR: None
Problem Summary: Auto partition creation can race with dynamic partition retention. After new partitions are added, the response builder reads the table partition map and partition info without holding the table metadata lock. If a retention drop removes one of those partitions in the same window, the response builder can observe a missing partition or partition item and hit a null pointer while constructing partition and tablet metadata. This change snapshots the required partition metadata under the table read lock, returns a normal error when the partition was already dropped before the snapshot, and keeps tablet location/cache/backend resolution work outside the table lock.
Release note
None
Check List (For Author)
Test: Manual test
./build.sh --fe
git diff --check -- fe/fe-core/src/main/java/org/apache/doris/service/FrontendServiceImpl.java
Behavior changed: Yes, concurrent retention drops now return a retry error instead of throwing an FE null pointer.
Does this need documentation: No