[fix](rowset) Keep the real error and drop failed writers in the rowset write path#66048
Open
airborne12 wants to merge 4 commits into
Open
[fix](rowset) Keep the real error and drop failed writers in the rowset write path#66048airborne12 wants to merge 4 commits into
airborne12 wants to merge 4 commits into
Conversation
…et write path
Three places lose information or state when a write fails.
SegmentCreator::flush() reset _flush_writer only on success:
RETURN_IF_ERROR(_flush_writer->flush());
_flush_writer.reset();
return Status::OK();
so after a failed flush the creator still holds the writer that just failed. add_block() had the same
shape -- a failed add_rows returned without discarding the writer, and the next call kept feeding
the broken one. Both now capture the status, drop the writer, and return the error.
SegmentFileCollection::close() used RETURN_IF_ERROR inside its loop, so a writer that failed to
close left every later writer unclosed. It now closes all of them and returns the first error.
_segcompaction_status was a std::atomic<int> holding only an error code, and each reader rebuilt a
fresh Status::Error<SEGCOMPACTION_FAILED>("... meet invalid state, error code: {}") from it, so the
original message and any more specific code were gone before the caller saw them. It is now an
AtomicStatus, which already exists in be/src/common/status.h, and the stored status is returned
as-is.
The two new tests drive the real failures rather than the refactor: one makes an early writer fail
to close and checks the later ones were still closed, the other injects a failure into
SegmentFlusher::_add_rows through a new TEST_SYNC_POINT and checks the creator allocates a fresh
segment instead of reusing the failed one.
airborne12
requested review from
csun5285,
eldenmoon,
gavinchou and
yiguolei
as code owners
July 25, 2026 13:22
Contributor
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
Member
Author
|
run buildall |
airborne12
added a commit
to airborne12/apache-doris
that referenced
this pull request
Jul 25, 2026
…ange The rowset write path fixes bundled here are not SNII: a failed SegmentCreator flush or add left the broken writer in place, SegmentFileCollection::close stopped at the first writer that failed to close, and segcompaction reported a generic SEGCOMPACTION_FAILED instead of its real status. No SNII or CommonGrams code calls any of it. They now live in apache#66048 against master, with the two failure-path tests that cover them. The six production files and segment_writer_mow_check_test.cpp go back to master here. row_ranges.h keeps is_empty() const -- that one looked like an unrelated tidy-up but segment_iterator.cpp calls it through a const RowRanges, so SNII needs it.
Contributor
TPC-H: Total hot run time: 29676 ms |
Contributor
TPC-DS: Total hot run time: 177502 ms |
Contributor
ClickBench: Total hot run time: 24.96 s |
Contributor
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
Issue Number: None Related PR: apache#66048 Problem Summary: Rowset write failures could be followed by retries that allocated additional segment IDs, while several early-return paths skipped data or index writer cleanup and could replace the original failure with a later close error. Latch the first write failure as terminal, make index close startup composable, and perform best-effort cleanup for normal, segcompaction, and vertical rowset writers while preserving the owner error. Failed rowset writes now terminate the writer and close all rowset output writers while preserving the original error. - Test: Unit Test - BE ASAN unit tests for segment/index collections, SegmentCreator terminal failures, segcompaction cleanup, and vertical final-flush cleanup - build-support/check-format.sh - Behavior changed: Yes. A SegmentCreator rejects subsequent writes after its first write failure and returns that original error. - Does this need documentation: No
Member
Author
|
run buildall |
### What problem does this PR solve? Issue Number: None Related PR: apache#66048 Problem Summary: The rowset writer fix introduced a dedicated header solely to preserve the first close error and a local FinalizedSegment type that only carried index-cache preload metadata. Follow the existing Doris explicit first-error pattern instead of expanding the common Status API or keeping a one-function header, reuse SegmentIndexFileCachePreloadTask, and gate index-cache preload on completion of all segment and index closes. ### Release note None ### Check List (For Author) - Test: Unit Test - 8 targeted BE ASAN unit tests for collection cleanup, terminal SegmentCreator failures, index-close preload gating, segcompaction cleanup, and vertical final-flush cleanup - build-support/check-format.sh - build-support/run-clang-tidy.sh --build-dir be/ut_build_ASAN (blocked by pre-existing types.h NOLINT and toolchain stddef.h errors) - Behavior changed: Yes. Index-cache preload is skipped when index finish-close fails; first-error propagation and cleanup behavior are preserved. - Does this need documentation: No
Contributor
TPC-H: Total hot run time: 29622 ms |
Contributor
TPC-DS: Total hot run time: 177366 ms |
Contributor
ClickBench: Total hot run time: 25.09 s |
### What problem does this PR solve? Issue Number: None Related PR: apache#66048 Problem Summary: Segment index cache preload task ownership was duplicated across rowset writer paths and leaked cache-specific task construction into vertical final flushing. Centralize buffering and gating in the existing loader module, preserve segment-internal index preload when no inverted index exists, and skip preload after any write or close failure while still closing all writers and returning the first error. ### Release note None ### Check List (For Author) - Test: Unit Test - 13 targeted ASAN BE unit tests - Behavior changed: Yes. Segment index cache preload runs only after all writes and file closes succeed. - Does this need documentation: No
Member
Author
|
run buildall |
Member
Author
|
/review |
Contributor
TPC-H: Total hot run time: 29742 ms |
Contributor
TPC-DS: Total hot run time: 177821 ms |
![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 result: request changes.
I found one blocking lifecycle gap: a delete-bitmap calculation failure bypasses terminal cleanup of the normal rowset data/index writers in both the changed local path and the parallel cloud path. See the inline comment for the concrete trigger and fix boundary.
Critical checkpoints:
- Goal and proof: The PR substantially implements terminal SegmentCreator failures, first-status preservation, all-writer close attempts, and post-close cache preload. It does not fully accomplish the cleanup goal because the new bitmap-error exit skips the normal writer ownership tree. The added tests do not cover this local/cloud failure path.
- Scope and clarity: The patch is focused on rowset writer failure handling and cache-preload lifecycle; no unrelated production changes were found.
- Concurrency: AtomicStatus publication and the queued/running segcompaction cancellation handoff were traced through their condition-variable and ownership interleavings. No additional race or deadlock was substantiated.
- Lifecycle, error handling, and data writes: Normal, vertical, and segcompaction close ordering otherwise preserves the owner/first error and attempts later closes. The accepted inline issue is the remaining failure exit that can leave buffered or asynchronously closing normal writers without the required cleanup/finish step.
- Configuration and special conditions: No new configuration was added. Existing mutable index-only cache behavior and the idempotent begin_close condition were reviewed; no separate configuration defect was confirmed.
- Compatibility and parallel paths: No storage-format, symbol, protocol, or rolling-upgrade change was introduced, and the more-specific Status propagation is intentional. CloudRowsetWriter is the missing functional parallel path covered by the inline finding.
- Tests and results: The targeted BE tests cover terminal write failures, first-error cleanup, vertical failure cleanup, fatal segcompaction status, and preload gating, but not the accepted bitmap-error cleanup path. Per the review-run contract, no local build or test was attempted. At submission time, compile, formatter, CheckStyle, and several checks pass; coverage is failing, while BE UT and several regression jobs remain pending.
- Observability and performance: Failure logs retain concrete Status values and identifiers, and no material hot-path regression was found. The unclosed remote-writer path can waste buffered/multipart resources, which is part of the blocking finding.
- Transactions, persistence, and FE/BE variables: No EditLog, persistent metadata schema, transaction commit protocol, or FE-BE variable propagation change is involved. Successful rowset visibility semantics are unchanged; the issue is cleanup after an already-failed write.
- User focus: No additional user-provided focus was specified.
The full 14-file authoritative diff and relevant upstream/downstream paths were reviewed. Two ledger rounds converged with all three round-2 agents reporting NO_NEW_VALUABLE_FINDINGS for head bb418e0.
| RETURN_IF_ERROR(_calc_delete_bitmap_token->wait()); | ||
| auto delete_bitmap_status = _calc_delete_bitmap_token->wait(); | ||
| if (!delete_bitmap_status.ok()) { | ||
| WARN_IF_ERROR(_synchronize_segcompaction(), |
Contributor
There was a problem hiding this comment.
[P1] Finish normal rowset writers on bitmap failure
This branch returns after cleaning only SegcompactionWorker, so the normal SegmentCreator ownership tree is skipped. A prior segment's delete-bitmap task can fail after a later segment is buffered; moreover, flushed inverted-index writers have only run begin_close() and still need InvertedIndexFileCollection::finish_close(). The same early return remains in CloudRowsetWriter::build(), where S3/packed-writer destruction is not equivalent to a successful explicit close and can leave multipart/buffered output abandoned. Please preserve delete_bitmap_status, mark/discard any buffered creator writer, and run a cleanup-only close for all normal data/index writers in both local and cloud build paths before returning. This should not reuse the success path unchanged because that may flush/preload or apply segcompaction/delete-bitmap conversion after the owner failure.
Contributor
ClickBench: Total hot run time: 24.97 s |
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
2 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:
Failures in the rowset write path were not handled consistently:
A failed
SegmentCreatorwrite could leave a failed writer reusable. A failedadd_block()or
flush()did not always discard the writer or preserve the failure. A later operation couldcontinue with invalid state or hide the original error.
File cleanup stopped at the first error. Segment and inverted-index writer collections used
early returns while closing multiple files. Once one writer failed, later writers could remain
open. Segcompaction output writers could also remain open after a fatal compaction error or a
delete-bitmap failure.
Segcompaction lost the original error.
_segcompaction_statusstored only an integer errorcode and reconstructed a generic
SEGCOMPACTION_FAILED, discarding the original status code andmessage.
Segment index cache preload was coupled to writer finalization and could run too early. It
could be triggered before every segment and inverted-index file had closed successfully, or
after an earlier write had already failed.
This PR makes
SegmentCreatorterminal after the first write failure and keeps returning thatoriginal status. It attempts to close all segment, inverted-index, and segcompaction output writers
while returning the first error. Segcompaction now preserves its full
Status. Segment index cachepreload is centralized in the existing loader module and runs only after all writes and file closes
succeed; disabled and local-rowset paths do not enqueue tasks. Rowsets without inverted indexes
still preload segment-internal index ranges such as short-key and footer data.
Release note
Fix rowset writer failure handling by preserving the original error, terminating a failed
SegmentCreator, closing all related file writers, and preloading segment indexes only after everywrite and file close succeeds.
Check List (For Author)
Test
Regression test
Unit Test
13 targeted ASAN BE unit tests cover:
SegmentCreator, preserving its first write error, and skipping preload afterwrite or close failure;
disabled, local-rowset, and final-flush-failure paths.
Manual test (add detailed scripts or steps below)
No need to test or manual test. Explain why:
Behavior changed:
No.
Yes.
The first rowset write or close error is preserved, cleanup continues for all related writers,
and segment index cache preload is skipped unless all writes and closes succeed. Callers may
now receive a more specific status instead of
SEGCOMPACTION_FAILED.Does this need documentation?
Check List (For Reviewer who merge this PR)