github-actions[bot] commented on code in PR #65905:
URL: https://github.com/apache/doris/pull/65905#discussion_r3630352804
##########
be/src/storage/segment/segment_writer.cpp:
##########
@@ -1030,18 +1030,6 @@ Status SegmentWriter::finalize(uint64_t*
segment_file_size, uint64_t* index_size
LOG(INFO) << "segment flush consumes a lot time_ns " <<
timer.elapsed_time()
<< ", segmemt_size " << *segment_file_size;
}
- // When the cache type is not ttl(expiration time == 0), the data should
be split into normal cache queue
Review Comment:
The retained test suite does not exercise this fix.
`CloudFileCacheWriteIndexOnlyTest` forces index-only mode, which gives segment
writers no cache builder and mocks the loader before the actual cache read, so
the deleted branch was unreachable even before this patch. The deterministic
out-of-order test from `028891cdc38` is removed by the head commit, meaning
this data-corruption path can be reintroduced while CI stays green. Please
retain an adapted passing test that blocks adjacent mock multipart uploads,
lets `finalize()` return, releases the later remote upload and waits until its
cache block is `DOWNLOADED`, then releases the earlier upload, waits for both
local copies and close, and compares cached bytes—including the cached-reader
path—with the assembled remote object.
##########
be/src/storage/segment/segment_writer.cpp:
##########
@@ -1030,18 +1030,6 @@ Status SegmentWriter::finalize(uint64_t*
segment_file_size, uint64_t* index_size
LOG(INFO) << "segment flush consumes a lot time_ns " <<
timer.elapsed_time()
<< ", segmemt_size " << *segment_file_size;
}
- // When the cache type is not ttl(expiration time == 0), the data should
be split into normal cache queue
- // and index cache queue
- if (auto* cache_builder = _file_writer->cache_builder(); cache_builder !=
nullptr &&
-
cache_builder->_expiration_time == 0 &&
-
config::is_cloud_mode()) {
- auto index_start = _index_file_cache_info.cache_start_offset();
- auto size = *index_size + *segment_file_size;
- auto holder = cache_builder->allocate_cache_holder(index_start, size,
_tablet->tablet_id());
- for (auto& segment : holder->file_blocks) {
-
static_cast<void>(segment->change_cache_type(io::FileCacheType::INDEX));
Review Comment:
The corruption comes from allocating an overlapping holder while multipart
cache copies are active, but deleting the whole block also removes the only
INDEX classification for ordinary non-TTL `SegmentWriter` cache writes.
`FileCacheAllocatorBuilder` creates those blocks as NORMAL, later
`is_index_data` reads only touch the block in its existing queue, and
`SegmentIndexFileCacheLoader` runs only under
`enable_file_cache_write_index_file_only` (default false). Thus
explicit/adaptive full-cache writes leave segment metadata in the NORMAL queue,
with a 1-day hot-data interval, instead of the separately-sized INDEX queue
with a 7-day interval; normal data churn can now evict metadata and force
remote reads. Please preserve the policy without recreating an unaligned live
holder—for example, wait on a separate local cache-copy completion barrier
before promoting only recorded ranges, or atomically promote existing blocks on
index access. `SegmentFileCollection::close()` alone is not s
uch a barrier when `enable_flush_file_cache_async` is true.
##########
be/src/storage/segment/segment_writer.cpp:
##########
@@ -1030,18 +1030,6 @@ Status SegmentWriter::finalize(uint64_t*
segment_file_size, uint64_t* index_size
LOG(INFO) << "segment flush consumes a lot time_ns " <<
timer.elapsed_time()
<< ", segmemt_size " << *segment_file_size;
}
- // When the cache type is not ttl(expiration time == 0), the data should
be split into normal cache queue
- // and index cache queue
- if (auto* cache_builder = _file_writer->cache_builder(); cache_builder !=
nullptr &&
Review Comment:
This deletion removes the finalize-time unaligned holder, but it does not
make the remaining S3 part holders unconditionally aligned.
`s3_write_buffer_size` is runtime-mutable, while divisibility by immutable
`file_cache_each_block_size` is checked only during
`ExecEnv::init_file_cache_factory()`. For example, updating 5 MiB to 5.5 MiB
before a writer makes part 2 start at 5.5 MiB. Because async cache flush
signals remote completion before its local copy, a published reader can first
install the aligned 5-6 MiB block; part 2 then intersects it and copies buffer
position zero at 5 MiB, shifting cached bytes by 0.5 MiB—the same corruption
class this PR targets. Please make the part size immutable, or validate aligned
updates and snapshot one validated size per `S3FileWriter`; include this
contract in the retained regression.
--
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]
