[fix](group commit) fix can not get a block queue#63722
Open
mymeiyi wants to merge 2 commits into
Open
Conversation
Contributor
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
Contributor
There was a problem hiding this comment.
Pull request overview
This PR addresses failures in group-commit stream load where the BE could not obtain a usable LoadBlockQueue under concurrency, by improving error handling around FE RPCs and adding resubmission/timeout logic for creating group-commit plan fragments. It also adds a new high-concurrency regression test intended to reproduce/guard the issue.
Changes:
- Improve RPC-status handling when aborting/committing group-commit transactions (avoid masking transport errors as FE result statuses).
- Add a background “create plan” worker + resubmission/timeout mechanism to reduce “can not get a block queue” occurrences during plan creation.
- Add a high-concurrency async group-commit stream load regression test.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| regression-test/suites/load_p0/stream_load/test_group_commit_stream_load_high_concurrency_async.groovy | New regression test stressing async group-commit under high concurrency. |
| be/src/load/group_commit/wal/wal_table.cpp | Return/LOG transport (RPC) errors distinctly from FE rollback result status. |
| be/src/load/group_commit/group_commit_mgr.h | Add new create-plan worker plumbing and make _need_commit atomic. |
| be/src/load/group_commit/group_commit_mgr.cpp | Implement create-plan resubmission worker, timeout handling, and improved commit/rollback RPC error handling. |
| be/src/common/config.h | Declare new config group_commit_create_plan_timeout_ms. |
| be/src/common/config.cpp | Define default for group_commit_create_plan_timeout_ms. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
mymeiyi
changed the title
Contributor
Author
|
/review |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
I found one blocking issue in the new group-commit plan creation worker. The new background thread does not initialize Doris thread context before executing BE code, which violates the BE runtime memory-tracking convention. Existing Copilot comments already covered the timeout comment mismatch, the add-load-id error message, and the p0 test size concern, so I did not duplicate those.
Checklist conclusions:
- Goal/test: The PR targets high-concurrency async group commit plan creation and adds a regression case, but the new worker thread setup is incomplete.
- Scope: The implementation is focused on group commit queue creation/retry, though it introduces a new manager thread.
- Concurrency: The new worker/condition-variable path is relevant; lock ordering looks consistent, but the worker thread lacks required thread-context initialization.
- Lifecycle:
GroupCommitMgr::stop()joins the new worker before shutting down the pool; no additional lifecycle blocker found. - Config: Adds a mutable timeout config with default/comment now aligned.
- Compatibility: No protocol/storage-format incompatibility found.
- Parallel paths: WAL rollback RPC handling was updated consistently in the touched paths.
- Tests: A stress regression test was added; existing review already noted the p0 size/flakiness risk.
- Observability: Existing logs are mostly sufficient for retries/failures.
- Transaction/persistence/data correctness: No additional transaction visibility or WAL recovery issue found in the changed logic.
- Performance: No additional performance blocker beyond the existing high-concurrency p0 test concern.
- User focus: No additional user-provided review focus was specified.
mymeiyi
force-pushed
the
fix-group-commit-get-queue
branch
2 times, most recently
from
9bbc402 to
47e1354
Compare
mymeiyi
force-pushed
the
fix-group-commit-get-queue
branch
from
47e1354 to
250bbcb
Compare
Contributor
Author
|
run buildall |
Contributor
TPC-H: Total hot run time: 31323 ms |
Contributor
TPC-DS: Total hot run time: 172487 ms |
Contributor
BE UT Coverage ReportIncrement line coverage Increment coverage report
|
Contributor
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
mymeiyi
force-pushed
the
fix-group-commit-get-queue
branch
from
5c6c86a to
3e7b817
Compare
Contributor
Author
|
run buildall |
Contributor
TPC-H: Total hot run time: 31954 ms |
Contributor
TPC-DS: Total hot run time: 172648 ms |
Contributor
BE UT Coverage ReportIncrement line coverage Increment coverage report
|
Contributor
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
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
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 #xxx
Related PR: #xxx
Problem Summary:
Release note
None
Check List (For Author)
Test
Behavior changed:
Does this need documentation?
Check List (For Reviewer who merge this PR)