github-actions[bot] commented on code in PR #67859:
URL: https://github.com/apache/doris/pull/67859#discussion_r3988313954
##########
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
Review Comment:
[P1] Preserve operations on rowsets created before this fix
Before this change, an empty local writer remained `OPENED`, so
`~LocalFileWriter()` aborted and deleted the `.idx` file. Affected rowsets
already on disk can therefore have an inverted-index schema but no V2/V3
container. This branch repairs only newly written rowsets: after upgrade,
`BetaRowset::link_files_to()` and `copy_files_to()` still unconditionally
link/copy the compound whenever the schema owns an index, so light schema
change and snapshot/copy operations fail on that persisted shape. Please add a
compatibility path for those legacy missing containers and seed a test with the
pre-fix on-disk state.
##########
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:
[P1] Keep HDFS close rejection from leaving a pending future
This generic branch now calls `HdfsFileWriter::close(true)` for an empty
remote index. HDFS installs `ASYNC_CLOSING` and its promise/future before the
fallible `submit_func()` call; if that submission is rejected, no task can
fulfill the promise, yet `finish_close()` and `~HdfsFileWriter()` both wait on
`future.get()`. Error unwinding can therefore hang the BE permanently. Please
make the HDFS rejection path establish a completed terminal result (the S3
synchronous fallback is one model), and cover rejected submission through an
empty `IndexFileWriter`.
##########
be/src/storage/task/index_builder.cpp:
##########
@@ -386,7 +386,14 @@ Status IndexBuilder::update_inverted_index_info() {
st = Status::Error<ErrorCode::INIT_FAILED>(
"debug point: reader init error");
})
- if (!st.ok() &&
!st.is<ErrorCode::INVERTED_INDEX_FILE_NOT_FOUND>()) {
+ // A missing container (the rowset predates every index) and an
+ // EMPTY one (the schema owns an index, but no logical index
had
+ // anything to write -- an all-NULL VARIANT column) both mean
the
+ // same thing here: there is nothing to carry over. Both leave
the
+ // reader un-inited, so get_all_directories() yields an empty
map
+ // and every requested index is built from the raw columns.
+ if (!st.ok() &&
!st.is<ErrorCode::INVERTED_INDEX_FILE_NOT_FOUND>() &&
+ !st.is<ErrorCode::INVERTED_INDEX_BYPASS>()) {
Review Comment:
[P2] Rebuild surviving indexes when accepting a zero-byte source
`INVERTED_INDEX_BYPASS` here proves only that the V2/V3 file has zero bytes;
it cannot distinguish the intended all-NULL container from a truncated compound
that previously held another index A. On this new success path the reader
contributes no directories, the output schema still retains A, and
`handle_single_rowset()` builds only the requested ALTER index B, so the
replacement container silently omits A (and can keep stale size metadata). The
changed unit test exercises this exact shape by truncating a populated `k1`
index before adding `k2`, but never verifies that `k1` survives. Please either
use a recognizable valid-empty representation or rebuild every output-schema
index missing from the source.
##########
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:
[P2] Replay a failed terminal close on retry
`S3FileWriter::close(false)` and `HdfsFileWriter::close(false)` set their
state to `CLOSED` before returning the async completion status. If the first
`finish_close()` reports an I/O failure, a retry now takes this shortcut and
returns `OK`, so the caller can treat an uncommitted empty artifact as
successfully closed. The new tests retry only successful closes. Please retain
and replay the terminal close status (and extend the failure test to call
`finish_close()` twice) rather than using `CLOSED` as a synonym for success.
--
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]
