github-actions[bot] commented on code in PR #66856:
URL: https://github.com/apache/doris/pull/66856#discussion_r3821972013
##########
be/src/storage/index/snii/snii_blob_staging_directory.cpp:
##########
@@ -49,15 +46,25 @@ class SniiBlobStagingDirectory::StagingIndexOutput final
try {
StagingIndexOutput::close();
} catch (const CLuceneError&) {
- // A destructor may not throw. Nothing here can fail anyway --
- // flushBuffer only appends to a vector -- but the base close() is
- // declared throwing, so the guard has to exist.
+ // A destructor may not throw. The normal success path closes
+ // explicitly so it can report this staging I/O failure.
}
}
- void close() override { BufferedIndexOutput::close(); }
+ void close() override {
+ if (_closed) {
+ return;
+ }
+ BufferedIndexOutput::close();
+ Status status = _file->finalize();
Review Comment:
[P2] Avoid fsyncing disposable ANN staging
This close now calls `StagedBlobFile::finalize()`, which unconditionally
`fsync`s the scratch file. `SniiCompoundWriter` immediately rereads every byte
into the real container, and `IndexFileWriter::begin_close()` later makes that
container durable with `_idx_v2_writer->close(true)`; the staging inode is then
unlinked and cannot recover an interrupted rowset. For GiB-scale HNSW/IVF
output this forces a complete durable scratch write and latency barrier before
the second full write, once per sub-file, and can reject a build on scratch
writeback even when the cached bytes could still be copied to the final writer.
Please give disposable ANN staging a non-durable seal-for-reading operation and
leave durability/error reporting to the final container close.
##########
be/src/storage/index/snii/snii_blob_staging_directory.cpp:
##########
@@ -151,12 +163,18 @@ void SniiBlobStagingDirectory::touchFile(const char*
/*name*/) {
lucene::store::IndexOutput* SniiBlobStagingDirectory::createOutput(const char*
name) {
DORIS_CHECK(name != nullptr);
// Same semantics as a filesystem directory: creating an existing name
- // truncates it. The buffer is replaced rather than cleared, so a blob
source
- // already taken over the old content keeps reading the old content
instead of
- // seeing it mutate underneath.
- auto buffer = std::make_shared<Buffer>();
- _files[name] = buffer;
- return _CLNEW StagingIndexOutput(std::move(buffer));
+ // truncates it. The file is replaced rather than reused, so a blob source
+ // already taken over the old content keeps reading the old content instead
+ // of seeing it mutate underneath.
+ std::unique_ptr<snii::bkd::StagedBlobFile> created;
+ Status status = snii::bkd::StagedBlobFile::create(name, &created);
+ if (!status.ok()) {
+ const std::string message = status.to_string();
+ _CLTHROWA(CL_ERR_IO, message.c_str());
+ }
+ auto file = std::shared_ptr<snii::bkd::StagedBlobFile>(std::move(created));
Review Comment:
[P2] Bound ANN staging before vertical rowset close
`StagedBlobFile::finalize()` deliberately keeps its fd open, and this map
owns it until `IndexFileWriter::begin_close()`. In vertical compaction,
`VerticalBetaRowsetWriter::_flush_columns()` finalizes ANN indexes for every
destination segment, but the rowset-wide `_idx_files.begin_close()` runs only
from `_close_file_writers()` after all column groups, segments, and post-merge
index work complete. A valid multi-segment/multi-ANN rowset therefore
accumulates one open fd per HNSW/IVF sub-file (two for IVF-on-disk) and a full
temporary copy of every destination ANN before the first source drains; enough
segments/indexes can hit `EMFILE` or fill the temp volume. This is distinct
from the existing ADD INDEX thread: here the producers are gone and the
directory owners in `_idx_files` retain the files before sealing. Please make
finalized sources safely close/reopen to bound descriptors and introduce a safe
phase/incremental-container lifecycle that can drain staged bytes earlier, with
a
vertical multi-destination lifecycle test.
##########
be/src/storage/index/ann/ann_index_writer.cpp:
##########
@@ -171,6 +171,17 @@ Status AnnIndexColumnWriter::_build_and_save(Int64
min_train_rows, Int64 effecti
// full-segment build buffer is released before saving the index.
PODArray<float> empty_buffered_vectors;
_buffered_vectors.swap(empty_buffered_vectors);
- return _vector_index->save(_dir.get());
+ Status status = _vector_index->save(_dir.get());
Review Comment:
[P2] Unwind ANN staging on later segment failures
This only discards the staging directory when `save()` itself fails. After a
successful ANN save, `SegmentWriter::finalize_columns_index()` can still fail
in bloom/key-index work, and `SegmentWriter::finalize()` can fail writing the
footer. `SegmentFlusher::_flush_segment_writer()` then returns before
`close_inverted_index()`, while `SegmentCreator::flush()` retains its failed
writer; the live rowset collection consequently keeps this finalized ANN
file/fd through `IndexFileWriter::_indices_dirs` (and pre-clear failures also
keep this producer's `_dir`). This is distinct from the existing save-failure
thread because serialization has succeeded. Please add a segment/index-writer
abort path that drops all SNII staging on every terminal finalize failure, with
a held-owner test injecting a post-ANN failure and checking the exact staged
path.
##########
be/test/storage/index/snii/snii_ann_container_test.cpp:
##########
@@ -412,6 +477,222 @@ class ScopedDebugPoints {
const bool _was_enabled;
};
+TEST_F(SniiAnnContainerTest, AnnStagingFinalizeFailureIsReturnedFromFinish) {
+ ScopedDebugPoints debug_points;
+ debug_points.enable("StagedBlobFile::finalize_error");
+
+ const std::string prefix = std::string(kTestDir) + "/finalize_failure_seg";
+ io::FileWriterPtr file_writer;
+ assert_ok(io::global_local_filesystem()->create_file(
+ InvertedIndexDescriptor::get_index_file_path_v2(prefix),
&file_writer));
+ IndexFileWriter writer(io::global_local_filesystem(), prefix,
"snii_ann_finalize_failure",
+ /*seg_id=*/0, InvertedIndexStorageFormatPB::SNII,
std::move(file_writer),
+ /*can_use_ram_dir=*/false,
+ /*tablet_id=*/9904);
+
+ Status finish_status = Status::OK();
+ ASSERT_NO_THROW({ finish_status = finish_ann_index(&writer, &_meta); });
+ EXPECT_FALSE(finish_status.ok());
+ EXPECT_NE(finish_status.to_string().find("injected blob staging finalize
failure"),
+ std::string::npos)
+ << finish_status.to_string();
+}
+
+TEST_F(SniiAnnContainerTest, FinalBufferedAppendFailureIsReturnedFromFinish) {
+ ScopedDebugPoints debug_points;
+ debug_points.enable("StagedBlobFile::append_error");
+
+ const std::string prefix = std::string(kTestDir) + "/append_failure_seg";
+ io::FileWriterPtr file_writer;
+ assert_ok(io::global_local_filesystem()->create_file(
+ InvertedIndexDescriptor::get_index_file_path_v2(prefix),
&file_writer));
+ IndexFileWriter writer(io::global_local_filesystem(), prefix,
"snii_ann_append_failure",
+ /*seg_id=*/0, InvertedIndexStorageFormatPB::SNII,
std::move(file_writer),
+ /*can_use_ram_dir=*/false,
+ /*tablet_id=*/9905);
+
+ Status finish_status = Status::OK();
+ ASSERT_NO_THROW({ finish_status = finish_ann_index(&writer, &_meta); });
+ EXPECT_FALSE(finish_status.ok());
+ EXPECT_NE(finish_status.to_string().find("injected blob staging append
failure"),
+ std::string::npos)
+ << finish_status.to_string();
+}
+
+TEST_F(SniiAnnContainerTest, IvfDataWriteFailuresAreReturnedFromFinish) {
Review Comment:
[P2] Exercise IVF-on-disk through the SNII reader
These new IVF cases only prove that staging write failures are returned. The
successful production-reader test uses `_meta` (HNSW, one `ann.faiss`), while
`IvfOnDiskSubFilesAreSealedInAscendingNameOrder` checks only the two
names/order; the separate IVF save/load test uses a `RAMDirectory`. A wrong
length/absolute extent or a bad `ann.ivfdata` handoff through
`StagedBlobFile::read_at` and `DorisCompoundReader` would therefore leave all
changed tests green and fail only when the new index is queried. Please seal
`_ivf_on_disk_meta` through `IndexFileWriter`, load it with `AnnIndexReader`,
and execute a query with checked neighbors.
##########
be/src/storage/index/snii/writer/snii_compound_writer.cpp:
##########
@@ -700,6 +708,7 @@ Status SniiCompoundWriter::finish() {
if (out_ == nullptr)
return Status::Error<ErrorCode::INVALID_ARGUMENT, false>("compound:
null file writer");
if (!failed_.ok()) {
+ release_all_blob_sources();
Review Comment:
[P2] Release blob callbacks at the poison boundary
This releases staged callbacks only if somebody calls `finish()` after the
writer has been poisoned. A native-BKD column can first register a callback
that is now the file's sole owner; if a later text index fails while writing
its physical sections, `poison()` makes the compound permanently unsealable and
`SegmentWriter::_write_inverted_index()` returns before
`close_inverted_index()`, so production never reaches this branch.
`SegmentCreator::flush()` also retains the failed writer, leaving the staged
file/fd pinned in its compound callback. This is distinct from the existing
in-`finish()` failure thread. Please release all blob sources on the first
transition in `poison()` (or from an explicit abort path) and test a registered
native-BKD blob followed by a later compound append failure while the outer
writer remains alive.
--
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]
