github-actions[bot] commented on code in PR #67347:
URL: https://github.com/apache/doris/pull/67347#discussion_r4006840445
##########
be/src/io/fs/packed_file_manager.cpp:
##########
@@ -431,17 +436,20 @@ Status
PackedFileManager::wait_for_packed_file_upload(PackedFileContext* packed_
return Status::OK();
}
-Status PackedFileManager::wait_upload_done(const std::string& path) {
- std::string packed_file_path;
- {
- std::lock_guard<std::mutex> global_lock(_global_index_mutex);
- auto it = _global_slice_locations.find(path);
- if (it == _global_slice_locations.end()) {
- return Status::InternalError("File not found in global index: " +
path);
- }
- packed_file_path = it->second.packed_file_path;
+Status PackedFileManager::wait_upload_done(const PackedSliceHandlePtr& handle)
{
+ if (handle == nullptr) {
+ return Status::InternalError("Missing packed slice handle");
}
+ // The upload already finished, so there is no need to look up the
PackedFileContext,
+ // which is only kept for a limited time after the upload.
+ auto upload_state = handle->upload_state();
Review Comment:
[P1] Keep ownership while a pending handle waits
`upload_state` is sampled only once. A normal synchronous close can see
`PENDING`, take `state.get()` from `_current_packed_files`, and then drop that
map's mutex. The background thread can subsequently rotate and finish the
context and, because `uploaded_file_retention_seconds` is mutable and accepts
0/negative values, cleanup can erase the last shared owner before the waiter
resumes. The waiter then dereferences freed mutex/CV storage at
`wait_for_packed_file_upload()`; if it instead misses every map, the stale
snapshot returns `Packed file not found` even though the handle is now
`UPLOADED`. The new recycled-context tests call this method only after terminal
publication, so they cannot cover this schedule. Please make the pending wait
itself retain shareable ownership (or wait on a primitive owned by the handle)
and re-read the handle state after lookup misses.
##########
be/src/io/fs/packed_file_writer.cpp:
##########
@@ -217,19 +217,29 @@ Status PackedFileWriter::_send_to_packed_manager() {
}
Slice data_slice(_buffer.data(), _buffer.size());
- RETURN_IF_ERROR(_packed_file_manager->append_small_file(_file_path,
data_slice, _append_info));
+ RETURN_IF_ERROR(_packed_file_manager->append_small_file(_file_path,
data_slice, _append_info,
+
&_packed_slice_handle));
+ if (_packed_slice_handle == nullptr) {
Review Comment:
[P2] Preserve an in-flight writer across a threshold decrease
This null handle is reachable without an internal invariant failure:
`small_file_threshold_bytes` is runtime-mutable, so `appendv()` can buffer a
file under the old value and `append_small_file()` can reject the same buffer
after the value is lowered. Returning an error here keeps the bytes in
`_buffer`, but `SegmentFileCollection::close()` has already marked the
collection closed and propagates the failure through
`CloudRowsetWriter::build()`, aborting that active rowset even though the
unused inner writer could persist it directly. The continuous-import test
lowers this setting only between manager calls, so it misses a writer spanning
the update. Please snapshot the pack/direct decision for the writer lifetime or
switch this retained buffer to the direct writer, and cover both close modes
with a synchronized threshold-change test.
##########
be/src/io/fs/packed_file_writer.cpp:
##########
@@ -176,8 +176,8 @@ Status PackedFileWriter::_close_sync() {
Status PackedFileWriter::_wait_packed_upload() {
DCHECK(!_is_direct_write);
// Only wait if we have data that was sent to packed manager
- if (_bytes_appended > 0 && _packed_file_manager != nullptr) {
- return _packed_file_manager->wait_upload_done(_file_path);
+ if (_bytes_appended > 0 && _packed_file_manager != nullptr &&
_packed_slice_handle != nullptr) {
Review Comment:
[P1] Wait for an accepted zero-length slice too
A non-null handle can be returned even when `_bytes_appended` is zero:
`_send_to_packed_manager()` submits the empty buffer, and the manager creates a
`PENDING` slice/context for it. This length guard then skips
`wait_upload_done()` in both synchronous close and asynchronous finish, marks
the wrapper `CLOSED`, and lets `CloudRowsetWriter` persist the still-pending
location before the packed object exists. This is production-reachable when a
V2 ANN segment is below `ann_index_build_min_segment_rows`: deleting the sole
index leaves an empty container that `IndexFileWriter` explicitly closes. A BE
exit before the manager's later idle rotation can therefore leave committed
metadata pointing to an object that was never uploaded, contrary to
`FileWriter::close(false)`'s empty-file persistence contract. Please wait
whenever a handle was accepted regardless of logical size, or route empty files
through the direct writer, and add sync/async empty-close coverage.
--
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]
