morningman commented on code in PR #68847:
URL: https://github.com/apache/doris/pull/68847#discussion_r4237266695
##########
be/src/io/file_factory.cpp:
##########
@@ -248,10 +262,12 @@ Result<io::FileReaderSPtr>
FileFactory::_create_file_reader_internal(
}
RETURN_IF_ERROR_RESULT(ExecEnv::GetInstance()->hdfs_mgr()->get_or_create_fs(
system_properties.hdfs_params, *fs_name, &handler));
+ // The reader's path drops the name node, so the cache key takes it
from here.
+ auto options = with_storage_in_cache_key(reader_options, *fs_name);
Review Comment:
I'd rather not key on the resolved HDFS configuration. To share a key, two
clusters need the same nameservice ID, the same path and the same modification
time, and HDFS keeps modification times in milliseconds. Files written
independently do not meet that. The case that does is a copy made with
preserved times (`distcp -pt`), which holds the same bytes, so the shared
blocks are still correct. S3 was different because its timestamps are whole
seconds, so files written by the same job in the same second matched. Keying on
the NameNode addresses would also give one cluster several keys whenever
catalogs spell its configuration differently (NameNode order, host name versus
IP, an added NameNode), so ordinary reads would lose cache hits to cover a case
that needs a deliberate copy.
##########
regression-test/suites/external_table_p0/tvf/test_http_tvf.groovy:
##########
@@ -317,6 +324,27 @@ suite("test_http_tvf", "p0") {
) order by id;
"""
+ // The same URL serving new content of the same length: an HTTP response
has no modification
+ // time for the file cache key, so the second query must not read blocks
cached by the first.
+ sql """ set enable_file_cache = true """
Review Comment:
Right: the p0 and External BE configurations leave `enable_file_cache` at
its default, false, so in those pipelines the case checks the HTTP read but not
the cache. #68797, which waits for this PR, turns the BE default on; from then
on the same case covers the stale-cache path in every pipeline. Until then it
was run locally with the BE cache on, where it fails when HTTP readers are
wrapped in the cache again (the second query returns `1, aaaa`), and
`HttpFileReaderTest.PlainResponseIsNotFileCachedEither` covers the reader
decision in BE UT.
--
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]
