Uh oh!
There was an error while loading. Please reload this page.
[fix](be) Validate stream load content length before group commit - #62110
Conversation
hello-stephen
commented
Apr 3, 2026
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
There was a problem hiding this comment.
Pull request overview
This PR fixes stream load group-commit pre-check behavior by validating/parsing Content-Length in a way that preserves the expected InvalidArgument handling for malformed header values, instead of allowing exceptions to bypass normal validation/error paths.
Changes:
- Add
try/catcharoundContent-Lengthparsing in_can_group_commit()to return aStatus::InvalidArgumenton malformed values. - Preserve existing negative-length handling while preventing
std::stollexceptions from escaping.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Uh oh!
There was an error while loading. Please reload this page.
### What problem does this PR solve? Issue Number: None Related PR: None Problem Summary: Group commit pre-check parsed Content-Length with std::stoll() before the regular request validation path, so malformed values could escape the normal InvalidArgument handling. ### Release note None ### Check List (For Author) - Test: No need to test (small BE validation fix; no completed automated test run in this environment) - Behavior changed: Yes (malformed Content-Length now returns a validation error instead of escaping the normal error path) - Does this need documentation: No
49eba17 to
1c56c21Comparedataroaring
commented
Apr 6, 2026
run buildall |
doris-robot
commented
Apr 6, 2026
TPC-H: Total hot run time: 28844 ms |
hello-stephen
commented
Apr 6, 2026
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
### What problem does this PR solve? Issue Number: None Related PR: apache#62110 Problem Summary: Cache the Content-Length header while validating group commit requests so the code avoids repeated lookups and reports the same parsed value on errors. ### Release note None ### Check List (For Author) - Test: No need to test (small refactor with no behavior change) - Behavior changed: No - Does this need documentation: No
liaoxin01
commented
Apr 8, 2026
run buildall |
hello-stephen
commented
Apr 8, 2026
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
liaoxin01
commented
Apr 15, 2026
/review |
There was a problem hiding this comment.
Found 1 blocking issue.
- Goal of the task: Partially accomplished. The PR intends to validate malformed
Content-Lengthbefore the group-commit pre-check, butgroup_commit=off_modestill bypasses the new validation, so the fix is incomplete. No test was added to prove the end-to-end behavior. - Modification size/focus: Yes. The patch is small and localized to
_can_group_commit(). - Concurrency: Not applicable for this change. I did not find a new lock/threading issue in the touched path.
- Lifecycle/static initialization: Not applicable.
- Configuration changes: None.
- Compatibility/incompatible change: None.
- Functionally parallel paths: No. There is still a parallel request path through
_handle_group_commit()'soff_modeearly return that skips the new validation and should be covered by the same check. - Special conditional checks: The
off_modeearly-return condition is now the critical hole; it bypasses the validation this PR adds. - Test coverage: Insufficient. No regression or unit test covers malformed or negative
Content-Length, especially withgroup_commit=off_mode. - Observability: Existing logs are sufficient for this small validation path.
- Transaction/persistence: Not involved.
- Data writes/modifications: There is still a correctness issue: invalid negative lengths can reach later stream/file setup instead of being rejected at the HTTP validation boundary.
- FE-BE variable passing: Not involved.
- Performance: Fine; the header reuse is a minor cleanup with no concern.
- Other issues: The blocking issue is described in the inline comment.
Overall opinion: request changes, because the PR does not fully close the invalid Content-Length hole it is trying to fix.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
…2110) Group commit pre-check parsed Content-Length before the normal request validation path, so malformed values could escape the expected InvalidArgument handling. - Add try/catch around Content-Length parsing in _can_group_commit() to return Status::InvalidArgument on malformed values. - Preserve existing negative-length handling while preventing std::stoll exceptions from escaping. - Cache the Content-Length header while validating group commit requests to avoid repeated lookups.
…ache#62110) Group commit pre-check parsed Content-Length before the normal request validation path, so malformed values could escape the expected InvalidArgument handling. - Add try/catch around Content-Length parsing in _can_group_commit() to return Status::InvalidArgument on malformed values. - Preserve existing negative-length handling while preventing std::stoll exceptions from escaping. - Cache the Content-Length header while validating group commit requests to avoid repeated lookups.
What problem does this PR solve?
Issue Number: None
Related PR: None
Problem Summary: Group commit pre-check parsed Content-Length before the normal request validation path, so malformed values could escape the expected InvalidArgument handling.
Release note
None
Check List (For Author)