airborne12 commented on code in PR #67859:
URL: https://github.com/apache/doris/pull/67859#discussion_r3988507843
##########
be/src/storage/index/index_file_writer.cpp:
##########
@@ -556,10 +561,11 @@ Status IndexFileWriter::finish_close() {
return Status::OK();
}
if (_indices_dirs.empty()) {
- // An empty file must still be created even if there are no indexes to
write
- if (dynamic_cast<io::StreamSinkFileWriter*>(_idx_v2_writer.get()) !=
nullptr ||
- dynamic_cast<io::S3FileWriter*>(_idx_v2_writer.get()) != nullptr ||
- dynamic_cast<io::PackedFileWriter*>(_idx_v2_writer.get()) !=
nullptr) {
+ // Second phase of the empty-file close begun in begin_close().
Skipping an
+ // already CLOSED writer keeps this idempotent: begin_close() may have
+ // closed synchronously, and a retried finish_close() must not send a
+ // second EOS or PUT a second empty object.
+ if (_idx_v2_writer != nullptr && _idx_v2_writer->state() !=
io::FileWriter::State::CLOSED) {
Review Comment:
This describes a caller that does not exist, against an idiom that predates
this PR.
**No caller retries `finish_close()` after a failure.** Every call site is a
single `RETURN_IF_ERROR`:
- `be/src/storage/rowset/segment_creator.cpp:127` — `SegmentFlusher::close()`
- `be/src/storage/rowset/beta_rowset_writer.cpp:292` —
`InvertedIndexFileCollection::finish_close()`, a loop that aborts on the first
error
- `be/src/storage/rowset/beta_rowset_writer.cpp:1269`
- `be/src/storage/task/index_builder.cpp` (x2) — both `return st`
**The `state() != CLOSED` guard is not new.** `origin/master` already has it
in `finish_close()` at `index_file_writer.cpp:553` (SNII branch) and `:567`
(the non-empty path), and `beta_rowset_writer.cpp` uses the same idiom at lines
198, 206, 440, 980 and 1262. This PR makes the empty path consistent with the
path right below it; if the shortcut were unsound it would be unsound for every
non-empty index file already.
The double `finish_close()` in the tests is asserting idempotence of a
*successful* close — that a retried finish sends no second `segment_eos` and
PUTs no second empty object — which is the property the fix needs. Extending it
to replay a terminal failure would be testing a contract no caller consumes.
--
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]
