airborne12 opened a new pull request, #66048:
URL: https://github.com/apache/doris/pull/66048
### What problem does this PR solve?
Issue Number: close #xxx
Related PR: #xxx
Problem Summary:
Three places in the rowset write path drop information or state when
something fails.
**1. A failed flush leaves the writer behind.** `SegmentCreator::flush()`
returned early on error:
```cpp
RETURN_IF_ERROR(_flush_writer->flush());
_flush_writer.reset();
return Status::OK();
```
`_flush_writer` is only reset on the success path, so after a failed flush
the creator still holds a
writer that has already failed. `SegmentCreator::add_block()` had the same
shape — a failed
`add_rows` returned without discarding the writer, so a retry kept feeding
the broken one.
**2. `SegmentFileCollection::close()` gives up on the first failure.** It
used `RETURN_IF_ERROR`
inside the loop over writers, so if writer *i* failed to close, writers
*i+1..n* were never closed
at all. Now every writer is closed and the first error is returned.
**3. Segcompaction reports a generic error instead of the real one.**
`_segcompaction_status` was a
`std::atomic<int>` holding only an error code, and every reader turned it
back into a fresh
`Status::Error<SEGCOMPACTION_FAILED>("... meet invalid state, error code:
{}")`. The original status
— message, and any more specific code — was gone by the time it reached the
caller. It is now an
`AtomicStatus` (already in `be/src/common/status.h`), so
`_segcompaction_status.status()` is
returned as-is.
### Release note
Fix the rowset writer discarding error detail and leaving failed segment
writers in place: a failed
`SegmentCreator` flush or add now discards the writer,
`SegmentFileCollection::close` closes every
writer instead of stopping at the first failure, and segcompaction
propagates its real error status
instead of a generic `SEGCOMPACTION_FAILED`.
### Check List (For Author)
- Test <!-- At least one of them must be included. -->
- [ ] Regression test
- [x] Unit Test
Two tests, both driving the real failure rather than asserting on
refactored code:
-
`SegmentFileCollectionTest.CloseAttemptsEveryWriterAfterFirstFailure` — makes
an early
writer fail to close and asserts the later ones were still closed,
and that the returned
status is the first error.
- `SegmentCreatorTest.StreamingAddFailureDiscardsWriterBeforeRetry` —
injects a failure into
`SegmentFlusher::_add_rows` through a new `TEST_SYNC_POINT` and
asserts the creator does not
reuse the failed writer on the next call.
- [ ] 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.
- [x] Yes.
Errors surfacing from the rowset write path may now be more specific
than
`SEGCOMPACTION_FAILED`, and a close that previously returned the first
failure while silently
skipping the remaining writers now closes all of them. Callers that
matched on
`SEGCOMPACTION_FAILED` specifically should be aware the underlying
code can now reach them.
- Does this need documentation?
- [x] No.
### 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 -->
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
