github-actions[bot] commented on code in PR #65058:
URL: https://github.com/apache/doris/pull/65058#discussion_r3569269716
##########
be/src/io/cache/block_file_cache.cpp:
##########
@@ -900,10 +901,21 @@ FileBlocks BlockFileCache::split_range_into_cells(const
UInt128Wrapper& hash,
while (current_pos < end_pos_non_included) {
current_size = std::min(remaining_size, _max_file_block_size);
remaining_size -= current_size;
- state = try_reserve(hash, context, current_pos, current_size,
cache_lock)
- ? state
- : FileBlock::State::SKIP_CACHE;
- if (state == FileBlock::State::SKIP_CACHE) [[unlikely]] {
+ auto block_state = state;
+ if (block_state != FileBlock::State::SKIP_CACHE &&
+ context.admit_cache_write_by_remote_scan_limiter) {
+ auto* limiter = context.remote_scan_cache_write_limiter;
+ DCHECK(limiter != nullptr);
+ if
(!limiter->try_admit_cache_write(static_cast<int64_t>(current_size))) {
+ block_state = FileBlock::State::SKIP_CACHE;
Review Comment:
`try_admit_cache_write()` only runs while this caller is creating a missing
cache cell. If another query has already inserted the `EMPTY` block, this query
can still win `get_or_set_downloader()` later and
`_read_remote_blocks_into_cache()` will append/finalize that block, but this
query's `RemoteScanCacheWriteLimiter` was never charged. That lets a limited
query populate cache with blocks first exposed by another query. Please move or
repeat admission at the successful downloader/write path, and fall back to a
non-caching path when the writer query is denied.
##########
be/src/storage/index/primary_key_index.cpp:
##########
@@ -96,26 +109,30 @@ Status
PrimaryKeyIndexBuilder::finalize(segment_v2::PrimaryKeyIndexMetaPB* meta)
Status PrimaryKeyIndexReader::parse_index(io::FileReaderSPtr file_reader,
const
segment_v2::PrimaryKeyIndexMetaPB& meta,
- OlapReaderStatistics*
pk_index_load_stats) {
+ OlapReaderStatistics*
pk_index_load_stats,
+ const io::IOContext* source_io_ctx) {
// parse primary key index
_index_reader.reset(new segment_v2::IndexedColumnReader(file_reader,
meta.primary_key_index()));
_index_reader->set_is_pk_index(true);
+ auto io_ctx = create_index_io_context(source_io_ctx, pk_index_load_stats);
RETURN_IF_ERROR(_index_reader->load(!config::disable_pk_storage_page_cache,
false,
- pk_index_load_stats));
+ pk_index_load_stats, &io_ctx));
_index_parsed = true;
return Status::OK();
}
Status PrimaryKeyIndexReader::parse_bf(io::FileReaderSPtr file_reader,
const
segment_v2::PrimaryKeyIndexMetaPB& meta,
- OlapReaderStatistics*
pk_index_load_stats) {
+ OlapReaderStatistics*
pk_index_load_stats,
+ const io::IOContext* source_io_ctx) {
// parse bloom filter
segment_v2::ColumnIndexMetaPB column_index_meta =
meta.bloom_filter_index();
segment_v2::BloomFilterIndexReader bf_index_reader(std::move(file_reader),
column_index_meta.bloom_filter_index());
+ auto io_ctx = create_index_io_context(source_io_ctx, pk_index_load_stats);
RETURN_IF_ERROR(bf_index_reader.load(!config::disable_pk_storage_page_cache,
false,
- pk_index_load_stats));
+ pk_index_load_stats, &io_ctx));
std::unique_ptr<segment_v2::BloomFilterIndexIterator> bf_iter;
RETURN_IF_ERROR(bf_index_reader.new_iterator(&bf_iter,
pk_index_load_stats));
RETURN_IF_ERROR(bf_iter->read_bloom_filter(0, &_bf));
Review Comment:
This fixes the load call, but the pages read by the iterators still lose the
same query context. `PrimaryKeyIndexReader::new_iterator()` constructs
`IndexedColumnIterator` without an `io_ctx`, and this Bloom-filter path also
calls `bf_index_reader.new_iterator(...)` without `&io_ctx`; those iterators
later read indexed-column data pages through a default non-query context.
Point-query and unique-key lookup misses can therefore still write PK-index/BF
pages outside `file_cache_query_limit_bytes`. Please thread the source
`IOContext` into both PK and BF iterators, not just the initial load.
##########
be/src/storage/segment/segment.cpp:
##########
@@ -957,13 +984,14 @@ Status Segment::traverse_column_meta_pbs(const
std::function<void(const ColumnMe
RETURN_IF_ERROR(_create_column_meta_once(&dummy_stats));
std::shared_ptr<SegmentFooterPB> footer_pb_shared;
RETURN_IF_ERROR(_get_segment_footer(footer_pb_shared, &dummy_stats));
- return _column_meta_accessor->traverse_metas(*footer_pb_shared, visitor);
+ return _column_meta_accessor->traverse_metas(*footer_pb_shared, visitor,
&dummy_stats);
}
Review Comment:
This helper is now context-aware internally, but this query path still
reaches it without any query context. `SchemaColumnDataSizesScanner` runs under
a `RuntimeState`, yet it calls `beta_rowset->load_segments(&segments)` which
opens segments with `nullptr`, then calls `segment->traverse_column_meta_pbs()`
which has no source `IOContext` for footer or V3 external metadata reads. A
`SELECT` from `information_schema.column_data_sizes` can still populate segment
metadata cache outside `file_cache_query_limit_bytes`. Please thread a query
`IOContext` from the scanner through segment loading and this traversal API.
##########
be/src/io/cache/cached_remote_file_reader.cpp:
##########
@@ -1266,6 +1283,7 @@ void CachedRemoteFileReader::prefetch_range(size_t
offset, size_t size, const IO
dryrun_ctx.query_id = nullptr;
dryrun_ctx.file_cache_stats = nullptr;
dryrun_ctx.file_reader_stats = nullptr;
+ dryrun_ctx.remote_scan_cache_write_limiter = nullptr;
Review Comment:
Clearing the limiter here avoids the async lifetime problem, but it also
drops the query's remote-only-on-miss state. Segment prefetch passes the query
`IOContext`, this copy nulls `remote_scan_cache_write_limiter` without setting
`file_cache_miss_policy`, and dry-run reads still append/finalize cache blocks
before the `is_dryrun` guards in `_read_remote_blocks_into_cache()`. With
`file_cache_query_limit_bytes=0`, foreground reads skip cache writes while
prefetch can still populate file cache. Please either keep the limiter alive
safely for the task, or force copied prefetch contexts from limited queries
into a non-writing remote-only/skip-cache mode.
--
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]
