github-actions[bot] commented on code in PR #68310:
URL: https://github.com/apache/doris/pull/68310#discussion_r4059811838
##########
be/test/io/cache/cached_remote_file_reader_test.cpp:
##########
@@ -178,6 +178,26 @@ class AsyncCachedRemoteFileReaderTest : public
BlockFileCacheTest {
} // namespace
+TEST_F(AsyncCachedRemoteFileReaderTest, sync_write_path_preserves_tablet_id) {
+ create_cache("cached_remote_reader_sync_write_tablet_id");
+ auto reader = create_reader(open_remote_file());
+
+ std::string result(64_kb, '\0');
+ FileCacheStatistics stats;
+ IOContext context;
+ context.file_cache_stats = &stats;
+ context.is_warmup = true;
+ size_t bytes_read = 0;
+ ASSERT_TRUE(
+ reader->read_at(0, Slice(result.data(), result.size()),
&bytes_read, &context).ok());
+ EXPECT_EQ(bytes_read, result.size());
+ EXPECT_EQ(result, std::string(result.size(), '0'));
+
+ const auto blocks = cache()->get_blocks_by_key(reader->_cache_hash);
+ ASSERT_EQ(blocks.size(), 1);
+ EXPECT_EQ(blocks.begin()->second->tablet_id(), 10086);
Review Comment:
[P2] Cover and restore TTL registration across restart
This assertion proves the ID only on the fresh in-memory cell, but the
stated TTL behavior is still lost after restart. Both metadata startup loading
and direct loading rebuild a positive `ctx.tablet_id` and call `add_cell()`
directly; the only production `register_tablet_id()` call is in
`split_range_into_cells()` for a new miss. Since the TTL manager's tablet set
is process-local, a fully cached tablet restored after its TTL changed or
expired while the BE was down is never reconciled until some later miss creates
another cell. Please register positive IDs from the restore/direct-load
lifecycle (accounting for manager initialization order) and extend this test
through persistence plus cache recreation/reconciliation.
##########
be/src/io/cache/cached_remote_file_reader.cpp:
##########
@@ -1078,6 +1078,7 @@ Status
CachedRemoteFileReader::_read_from_indirect_cache(size_t offset, Slice re
s_align_size(offset + already_read, bytes_req - already_read,
size());
CacheContext cache_context(io_ctx);
cache_context.stats = &stats;
+ cache_context.tablet_id = _tablet_id;
Review Comment:
[P2] Preserve the no-tablet sentinel for external readers
`_read_from_indirect_cache()` is also the default path for external scans:
`FileFactory::get_reader_options()` enables their cache without changing
`FileReaderOptions::tablet_id`, whose unset value is `-1`. Before this line,
`CacheContext` kept its `0` no-tablet value. Copying `-1` makes each external
miss persist a `BlockMetaKey` in a new namespace and, because
`split_range_into_cells()` registers every nonzero ID, enqueue an invalid
tablet for TTL maintenance and `get_tablet_meta(-1)`. It can also miss
pre-upgrade external metadata keyed by `0` during direct startup loading.
Please normalize non-Doris/unset IDs back to `0` consistently (including the
async path), and cover an external cached reader in the test.
--
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]
