airborne12 commented on code in PR #67859:
URL: https://github.com/apache/doris/pull/67859#discussion_r3988509887
##########
be/src/storage/index/index_file_writer.cpp:
##########
@@ -508,10 +507,16 @@ Status IndexFileWriter::begin_close() {
return _idx_v2_writer->close(true);
}
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) {
+ // A schema that owns an index file always gets one, even when no
logical
+ // index had anything to write (an all-NULL VARIANT column extracts no
+ // subcolumn, so no directory is ever opened). The file is committed by
+ // close(), not by create_file(): S3 turns a zero-byte writer into an
empty
+ // object, StreamSink sends segment_eos, and LocalFileWriter's
destructor
+ // ABORTS -- and deletes -- a writer it was never asked to close.
Dispatch
+ // through FileWriter rather than naming implementations: the old
whitelist
+ // silently dropped LocalFileWriter and HdfsFileWriter, and every new
+ // implementation would have had to remember to add itself here.
+ if (_idx_v2_writer != nullptr && _idx_v2_writer->state() !=
io::FileWriter::State::CLOSED) {
Review Comment:
The asymmetry you describe is real, but it is a pre-existing
`HdfsFileWriter` defect that this PR barely touches.
`HdfsFileWriter::close(non_block=true)` sets `_state = ASYNC_CLOSING` and
installs the promise/future *before* `submit_func()`, then returns the submit
status with no fallback (`be/src/io/fs/hdfs_file_writer.cpp`).
`S3FileWriter::close()` does have the fallback:
```cpp
if (!submit_status.ok()) {
s3_file_writer_async_close_queuing << -1;
LOG(WARNING) << "failed to submit async close for " << ... << ",
fallback to sync close, ...";
```
So the HDFS writer should mirror that. But this PR is not what makes it
reachable: `VerticalSegmentWriter::finalize_footer()` already calls
`_file_writer->close(true)` on the **segment .dat writer for every segment
written** (`be/src/storage/segment/vertical_segment_writer.cpp:802`), and
`inverted_index_fs_directory.cpp:459` is another existing `close(true)` site.
On an HDFS-backed vault the rejected-submission hang is therefore reachable on
every segment flush today, orders of magnitude more often than through an empty
index file — which additionally requires an all-NULL VARIANT index on that
segment.
Fixing `HdfsFileWriter` to establish a terminal result on submission
rejection is worth doing and I am happy to send it, but it belongs in its own
PR against `hdfs_file_writer.cpp` (and it would want an HDFS-side inject point;
today only S3 has `S3FileWriter.close.submit_async_close.inject_error`). Gating
this fix on it would leave the empty index file being deleted on local storage
in the meantime.
--
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]
