[feat](group commit) add group_commit_mode in table property#61242
[feat](group commit) add group_commit_mode in table property#61242dataroaring merged 3 commits intoapache:masterfrom
Conversation
mymeiyi
requested review from
CalvinKirs,
dataroaring,
gavinchou and
w41ter
as code owners
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
There was a problem hiding this comment.
Pull request overview
Adds a new group_commit_mode table property so group-commit behavior (async/sync/off) can be driven by table metadata when the group_commit HTTP header is absent, and wires this through FE/BE RPCs plus regression coverage.
Changes:
- Introduce
group_commit_modeproperty parsing/storage and SHOW CREATE TABLE output (hidden whenoff_mode). - Extend stream load logic (FE HTTP v2 + BE stream load) to consult table property when
group_commitheader is not provided. - Add regression tests for CREATE/ALTER/SHOW behavior and stream-load behavior with/without header override.
Reviewed changes
Copilot reviewed 18 out of 18 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| regression-test/suites/load_p0/stream_load/test_group_commit_stream_load.groovy | Adds stream-load regression cases for table-property-driven group commit and header override. |
| regression-test/suites/insert_p0/insert_group_commit_into.groovy | Adds DDL/ALTER/SHOW CREATE coverage for group_commit_mode. |
| gensrc/thrift/FrontendService.thrift | Adds use_table_group_commit_mode request flag and table_group_commit_mode response field for loadTxnBegin. |
| gensrc/proto/cloud.proto | Adds group_commit_mode to tablet meta update proto for cloud. |
| fe/fe-core/src/main/java/org/apache/doris/service/FrontendServiceImpl.java | Supports returning table group-commit mode (skipping begin txn) when requested. |
| fe/fe-core/src/main/java/org/apache/doris/nereids/trees/plans/commands/info/ModifyTablePropertiesOp.java | Validates group_commit_mode ALTER TABLE property changes. |
| fe/fe-core/src/main/java/org/apache/doris/httpv2/rest/LoadAction.java | Uses table property when group_commit header is absent (redirect / block logic). |
| fe/fe-core/src/main/java/org/apache/doris/datasource/InternalCatalog.java | Parses/stores group_commit_mode during table creation. |
| fe/fe-core/src/main/java/org/apache/doris/common/util/PropertyAnalyzer.java | Adds constants + parsing/validation for group_commit_mode. |
| fe/fe-core/src/main/java/org/apache/doris/cloud/alter/CloudSchemaChangeHandler.java | Allows and propagates group_commit_mode updates to cloud tablet meta. |
| fe/fe-core/src/main/java/org/apache/doris/catalog/TableProperty.java | Stores/reads group_commit_mode in table properties. |
| fe/fe-core/src/main/java/org/apache/doris/catalog/OlapTable.java | Exposes get/setGroupCommitMode() on OlapTable. |
| fe/fe-core/src/main/java/org/apache/doris/catalog/Env.java | Prints group_commit_mode in SHOW CREATE TABLE only when not off_mode. |
| fe/fe-core/src/main/java/org/apache/doris/alter/SchemaChangeHandler.java | Allows group_commit_mode as a modifiable table property. |
| be/src/service/http/action/stream_load.h | Adds helper declaration for group-commit eligibility check. |
| be/src/service/http/action/stream_load.cpp | Refactors group-commit header handling; uses ctx->group_commit_mode. |
| be/src/load/stream_load/stream_load_executor.cpp | Adds FE lookup path for table group_commit_mode via loadTxnBegin. |
| be/src/load/stream_load/stream_load_context.h | Adds group_commit_mode to stream load context. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
regression-test/suites/load_p0/stream_load/test_group_commit_stream_load.groovy
Show resolved
Hide resolved
regression-test/suites/load_p0/stream_load/test_group_commit_stream_load.groovy
Show resolved
Hide resolved
| if (!partial_columns && !partitions && !temp_partitions && !ctx->two_phase_commit && | ||
| !update_mode) { | ||
| if (!config::wait_internal_group_commit_finish && !ctx->label.empty()) { | ||
| return Status::InvalidArgument("label and group_commit can't be set at the same time"); | ||
| } |
There was a problem hiding this comment.
In _can_group_commit, the label and group_commit can't be set at the same time check runs even when the client did not set the group_commit header (i.e., group_commit_header is empty). This can reject normal (non-group-commit) stream loads that specify a label on tables whose group_commit_mode is off, which is a backward-incompatible behavior change. Consider skipping the label conflict check when group_commit_header is empty, and instead enforce/clear label only after the table property lookup confirms group commit will be used (e.g., in begin_txn when table_group_commit_mode is returned).
fe/fe-core/src/main/java/org/apache/doris/common/util/PropertyAnalyzer.java
Show resolved
Hide resolved
mymeiyi
force-pushed
the
table-group-commit-mode-6
branch
2 times, most recently
from
8e07520 to
c0dc62a
Compare
|
run buildall |
Cloud UT Coverage ReportIncrement line coverage Increment coverage report
|
|
/review |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review Summary for PR #61242 — [feat](group commit) add group_commit_mode in table property
Overview
This PR adds a group_commit_mode table property that allows tables to specify a default group commit mode (off_mode, async_mode, sync_mode). When a stream load is issued without an explicit group_commit HTTP header, the BE queries the FE for the table's property and uses it. The FE changes (PropertyAnalyzer, TableProperty, OlapTable, SchemaChangeHandler, CloudSchemaChangeHandler, FrontendServiceImpl) follow existing patterns correctly. EditLog persistence works via the direct-map-read pattern (no build* method needed, consistent with group_commit_interval_ms and group_commit_data_bytes).
Critical Checkpoint Conclusions
-
Goal & correctness: The feature goal is achieved for the
StreamLoadActionpath. The FE property management (CREATE TABLE, ALTER TABLE, SHOW CREATE TABLE, EditLog persistence/replay, Cloud mode support) is implemented correctly and consistently with existing group_commit properties. -
Modification scope: The PR is focused on adding a single table property and wiring it through BE stream load. However, there are missing parallel paths (see below).
-
Concurrency: The
table.getGroupCommitMode()call inFrontendServiceImpl.loadTxnBeginImplreads the property without holding a table read lock. This is consistent with the existing pattern for other table property reads in this method and is acceptable since the worst case is reading a slightly stale value (the property value is a simple String reference). -
Lifecycle management: No special lifecycle concerns. The property is stored in the
propertiesmap and follows the same pattern asgroup_commit_interval_ms. -
Configuration items: The
group_commit_modeproperty is dynamically changeable via ALTER TABLE — changes are immediately visible without restart. This is correct. -
Incompatible changes: New Thrift field
use_table_group_commit_mode(field 17) andtable_group_commit_mode(field 5) are optional, so rolling upgrades are safe. New proto fieldgroup_commit_mode(field 15) is also optional. -
Parallel code paths: Issue found —
HttpStreamAction(http_stream.cpp) is not updated to support table-level group_commit_mode. See inline comment. -
Test coverage: Regression tests cover CREATE TABLE, ALTER TABLE, SHOW CREATE TABLE, invalid values, stream load with/without header, and header override. Missing: test for label conflict with table property, and no test for
HttpStreamActionpath. -
Observability: The group_commit_mode is logged in the stream load request log (line 260), which is sufficient.
-
Transaction/persistence: EditLog persistence is handled correctly via the existing
modifyTableProperties→logModifyTablePropertiesflow. No separatebuild*()needed because the getter reads directly from the properties map. -
FE-BE variable passing: The new Thrift fields are correctly set in the stream load
begin_txnpath. However, this only coversStreamLoadAction, notHttpStreamAction. -
Performance: No performance concerns. The table property lookup is a simple HashMap
getOrDefault.
Issues Found
See inline comments for details. Summary:
- [High]
_can_group_commitlabel check may reject legitimate labeled stream loads on tables with group_commit_mode set - [Medium]
HttpStreamActionparallel path not updated for table-level group_commit_mode - [Low] Missing
__issetcheck for Thrift result field - [Low]
_check_wal_spacecalled withbody_bytes=0in table-property path (effectively a no-op)
| @@ -933,17 +940,43 @@ Status StreamLoadAction::_handle_group_commit(HttpRequest* req, | |||
| if (!config::wait_internal_group_commit_finish && !ctx->label.empty()) { | |||
| return Status::InvalidArgument("label and group_commit can't be set at the same time"); | |||
| } | |||
There was a problem hiding this comment.
[High] Label check incorrectly rejects labeled stream loads on tables with group_commit_mode property
When the user does NOT set the group_commit HTTP header (empty group_commit_header), but the table has group_commit_mode=async_mode/sync_mode, this code path is still reached via _handle_group_commit → _can_group_commit. If the user provides a label (which is normal for stream loads), this returns InvalidArgument("label and group_commit can't be set at the same time").
The problem: the user never explicitly requested group commit — they just have a table property. This is a behavioral regression because existing users who always set labels would suddenly get errors after someone sets the table property.
Suggestion: Either skip this check when group_commit_header is empty (table-property path), or move the label conflict check to after the FE responds confirming the table actually uses group commit. For example:
if (!group_commit_header.empty() && !config::wait_internal_group_commit_finish && !ctx->label.empty()) {
return Status::InvalidArgument("label and group_commit can't be set at the same time");
}This ensures only explicit header-based group commit rejects labels, while the table-property path allows labels (and falls through to a normal non-group-commit load when a label is present).
| ctx->group_commit = true; | ||
| } else { | ||
| // use table property to decide group commit or not | ||
| ctx->group_commit_mode = ""; |
There was a problem hiding this comment.
[Medium] Parallel code path: HttpStreamAction not updated
The HttpStreamAction (http_stream.cpp) has its own _handle_group_commit method that does NOT support the table-property group commit mode. It treats an empty group_commit header as "no group commit" and returns immediately. Unlike StreamLoadAction, it doesn't call begin_txn (the transaction is opened by FE via NativeInsertStmt), so the mechanism used here (empty group_commit_mode → begin_txn queries FE for table property) doesn't directly apply.
However, the feature description says "For stream load, if it not set group_commit header, use the table property as the group commit mode". This should apply to both stream load endpoints. Consider either:
- Adding table-property support to
HttpStreamActionin this PR, or - Documenting this as a known limitation and filing a follow-up issue.
| ctx->begin_txn_cost_nanos = MonotonicNanos() - begin_txn_start_time; | ||
| if (ctx->group_commit) { | ||
| RETURN_IF_ERROR(_check_wal_space(ctx->group_commit_mode, ctx->body_bytes)); | ||
| } |
There was a problem hiding this comment.
[Low] _check_wal_space called with body_bytes=0
At this point in _on_header, ctx->body_bytes is initialized to 0 (set at line 304). The body hasn't been received yet. So _check_wal_space(ctx->group_commit_mode, ctx->body_bytes) is called with content_length=0, which always passes the load_size_smaller_than_wal_limit(0) check.
This means the WAL space check is effectively a no-op for the table-property group commit path. The explicit header path has a proper check in _can_group_commit (line 943) using the Content-Length header value.
Consider using the Content-Length from the HTTP header instead:
if (ctx->group_commit) {
int64_t content_length = http_req->header(HttpHeaders::CONTENT_LENGTH).empty()
? 0 : std::stoll(http_req->header(HttpHeaders::CONTENT_LENGTH));
RETURN_IF_ERROR(_check_wal_space(ctx->group_commit_mode, content_length));
}
github-actions
bot
added
the
approved
|
PR approved by at least one committer and no changes requested. |
|
PR approved by anyone and no changes requested. |
BE UT Coverage ReportIncrement line coverage Increment coverage report
|
mymeiyi
force-pushed
the
table-group-commit-mode-6
branch
from
c0dc62a to
fc3afe7
Compare
github-actions
bot
removed
the
approved
|
run buildall |
FE UT Coverage ReportIncrement line coverage |
TPC-H: Total hot run time: 27972 ms |
TPC-DS: Total hot run time: 153770 ms |
BE UT Coverage ReportIncrement line coverage Increment coverage report
|
mymeiyi
force-pushed
the
table-group-commit-mode-6
branch
from
fc3afe7 to
28dfdf8
Compare
|
run buildall |
Cloud UT Coverage ReportIncrement line coverage Increment coverage report
|
TPC-H: Total hot run time: 27973 ms |
TPC-DS: Total hot run time: 152765 ms |
BE UT Coverage ReportIncrement line coverage Increment coverage report
|
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
github-actions
bot
added
the
approved
|
PR approved by at least one committer and no changes requested. |
…61242) ### What problem does this PR solve? Issue Number: close #xxx Related PR: #xxx Problem Summary: 1. Create table support set `group_commit_mode` table property ``` CREATE TABLE ... PROPERTIES( "group_commit_mode" = "async_mode" ); ``` 2. Support alter this property ``` ALTER TABLE ... SET ("group_commit_mode" = "off_mode"); ``` 3. Show create table shows this property if its value is not `off_mode` 4. For stream load, if it not set `group_commit` header, use the table property as the group commit mode; if it set `group_commit` header, use the header value. 5. doc: apache/doris-website#3465 ### Release note None ### Check List (For Author) - Test <!-- At least one of them must be included. --> - [ ] Regression test - [ ] Unit Test - [ ] Manual test (add detailed scripts or steps below) - [ ] No need to test or manual test. Explain why: - [ ] This is a refactor/code format and no logic has been changed. - [ ] Previous test can cover this change. - [ ] No code files have been changed. - [ ] Other reason <!-- Add your reason? --> - Behavior changed: - [ ] No. - [ ] Yes. <!-- Explain the behavior change --> - Does this need documentation? - [ ] No. - [ ] Yes. <!-- Add document PR link here. eg: apache/doris-website#1214 --> ### Check List (For Reviewer who merge this PR) - [ ] Confirm the release note - [ ] Confirm test cases - [ ] Confirm document - [ ] Add branch pick label <!-- Add branch pick label that this PR should merge into -->
What problem does this PR solve?
Issue Number: close #xxx
Related PR: #xxx
Problem Summary:
group_commit_modetable propertyoff_modegroup_commitheader, use the table property as the group commit mode; if it setgroup_commitheader, use the header value.Release note
None
Check List (For Author)
Test
Behavior changed:
Does this need documentation?
Check List (For Reviewer who merge this PR)