morningman opened a new pull request, #68847:
URL: https://github.com/apache/doris/pull/68847
### What problem does this PR solve?
Issue Number: None
Related PR: #68797, #66932
Problem Summary:
**In short.** The BE file cache can return the bytes of a different file for
two kinds of external reads. An `http()` URL whose content changes keeps being
served from blocks cached from the old content. Two object storage endpoints,
or two HDFS clusters, that hold the same path with the same modification time
share cache blocks. Both happen on master whenever a session turns on
`enable_file_cache`, and on branch-4.x, where the cache is on by default.
#68797 turns the cache on by default on master and is blocked on these two
cases. This PR keeps HTTP responses out of the file cache and adds the storage
to the cache key of external files.
**Background.** `CachedRemoteFileReader` keys an external file as
`path:mtime`: the path its reader reports plus the modification time from the
file listing. The key is meant to change whenever the file does. `FileFactory`
wraps S3, HDFS and HTTP readers in the cache when the query enables it. #66932
already exempted chunked HTTP responses (the `cdc_stream` table function),
whose content is a stream.
**The problem, and what it cost.**
- **HTTP has no version.** `HttpTableValuedFunction` lists a URL with
modification time 0, so every response of that URL gets the key `url:0`. When
the content behind the URL changes, a later query reads blocks cached from the
earlier content. If the length did not change, nothing fails and the query
silently returns the old rows; if it did, old and new bytes can be mixed.
- **The key does not name the storage.** An S3 reader reports
`s3://bucket/key`, without the endpoint. An HDFS reader reports the path with
the name node stripped (`convert_path`). Two S3-compatible endpoints (for
example two MinIO deployments) holding the same bucket and key, or two HDFS
clusters holding the same path, get the same key as soon as the modification
times match. S3 reports them in seconds, so files written by the same job in
the same second match. A query on one storage can then read blocks of the other
storage's file.
**How this PR fixes it.**
- `FileFactory` no longer wraps HTTP readers in the file cache. This covers
every HTTP source read through `FileFactory`, including `hf://` URLs, and
replaces the chunked-response check from #66932, which this rule includes.
- `FileReaderOptions` gets `fs_identity`, the storage an external file is
read from. `FileFactory` sets it to the S3 endpoint (`AWS_ENDPOINT`) for S3
readers and to the name node (`fs_name`) for HDFS readers.
`CachedRemoteFileReader` keys external files as `fs_identity:path:mtime`;
readers that do not set it, such as broker reads, keep `path:mtime`.
**Results.**
| Case | Before | After |
|---|---|---|
| `http()` URL whose content was replaced, file cache on | second query can
return rows of the old content | always reads the current content; HTTP data is
not cached |
| Same bucket, key and mtime on two S3-compatible endpoints | one cache
entry, shared | one entry per endpoint |
| Same path and mtime on two HDFS clusters | one cache entry, shared | one
entry per name node |
| Existing cached blocks of external S3 / HDFS files after upgrade | - | not
found under the new key; the first reads go to the storage again and the old
blocks age out |
**Classes, and how they call each other.**
```
FileScanner / format readers
└─ FileFactory::create_file_reader(system_properties, file_description,
reader_options)
├─ FILE_S3 : options.fs_identity = s3_conf.client_conf.endpoint ─┐
├─ FILE_HDFS : options.fs_identity = fs_name ──────────────────────┤
│
create_cached_file_reader(reader, options)
│ └─
CachedRemoteFileReader::_init_external_table_cache
│ key =
fs_identity:path:mtime (path:mtime if empty)
└─ FILE_HTTP : HttpFileReader, not wrapped in the file cache
```
### Release note
The file cache no longer keeps data read over HTTP (the `http()` table
function, including `hf://` URLs), because an HTTP source has no modification
time to tell versions apart. Cached data of external files on S3-compatible
storage and HDFS is now kept per endpoint or name node; after upgrading, data
cached by an earlier version is not reused for these files and is evicted over
time.
### Check List (For Author)
- Test <!-- At least one of them must be included. -->
- [x] Regression test
- [x] Unit Test
- [ ] Manual test (add detailed scripts or steps below)
- [ ] No need to test or manual test. Explain why:
- [ ] This is a refactor/code format and no logic has been changed.
- [ ] Previous test can cover this change.
- [ ] No code files have been changed.
- [ ] Other reason <!-- Add your reason? -->
Regression test: `test_http_tvf` adds `replaced_before` /
`replaced_after`. With `enable_file_cache = true`, the suite's local HTTP
server replaces the file behind one URL with content of the same length between
two queries, and the second query must return the new row. Run locally with the
BE file cache on, it passes. With HTTP readers wrapped in the file cache again,
the second query returns the old row (`1, aaaa` instead of `2, bbbb`). Only the
two new blocks of `test_http_tvf.out` were taken from the generated file: a
local regeneration also rewrites `sql08` (local time zone) and drops two blocks
the suite no longer produces.
BE unit tests:
- `HttpFileReaderTest.PlainResponseIsNotFileCachedEither`: an HTTP
reader is not wrapped in the file cache even when the query asks for the cache.
-
`AsyncCachedRemoteFileReaderTest.external_cache_key_tells_storages_apart`: the
same path and modification time on two storages get different keys; a reader
without an identity keeps `path:mtime`.
-
`AsyncCachedRemoteFileReaderTest.file_factory_keys_s3_files_by_endpoint`:
readers that `FileFactory` builds for two S3 endpoints get different keys.
`run-be-ut.sh --run
--filter='HttpFileReaderTest.*:AsyncCachedRemoteFileReaderTest.*'` passed. Each
new case fails when its part of the change is reverted: HTTP readers wrapped in
the cache again (SEGV in `FileCacheFactory::get_by_path`, since that test has
no cache), the S3 reader built without its endpoint, or the key built without
the identity. The HDFS wiring has no unit test, because creating an HDFS reader
in BE UT would start a JVM.
- Behavior changed:
- [ ] No.
- [x] Yes. HTTP reads no longer use the file cache. External S3 / HDFS
files are cached under a new key, so blocks cached before the upgrade are not
reused.
- Does this need documentation?
- [x] No.
- [ ] Yes. <!-- Add document PR link here. eg:
https://github.com/apache/doris-website/pull/1214 -->
### Check List (For Reviewer who merge this PR)
- [ ] Confirm the release note
- [ ] Confirm test cases
- [ ] Confirm document
- [ ] Add branch pick label <!-- Add branch pick label that this PR should
merge into -->
--
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]
