wangyong9999 commented on code in PR #411:
URL: https://github.com/apache/paimon-cpp/pull/411#discussion_r4218963165
##########
src/paimon/core/utils/objects_file.h:
##########
@@ -255,23 +255,175 @@ Result<std::unique_ptr<InputStream>>
ObjectsFile<T>::OpenForRead(
return file_system_->Open(file_path);
}
+// Only in-progress loads are retained here. Readers sharing a cache and path
share one load,
+// including across ObjectsFile instances; unrelated files load without
holding the registry lock.
template <typename T>
-Result<MemorySegment> ObjectsFile<T>::ReadFileSegment(
- const std::string& file_path, const std::optional<int64_t>& file_size)
const {
- PAIMON_ASSIGN_OR_RAISE(std::unique_ptr<InputStream> input_stream,
- OpenForRead(file_path, file_size));
- PAIMON_ASSIGN_OR_RAISE(int64_t input_length, input_stream->Length());
+Result<std::shared_ptr<CacheValue>> ObjectsFile<T>::LoadCacheEntry(
+ const std::shared_ptr<Cache>& cache, const std::string& path,
+ const std::function<Result<std::shared_ptr<CacheValue>>()>& load) {
+ using LoadKey = std::pair<std::shared_ptr<Cache>, std::string>;
+ using CacheResult = Result<std::shared_ptr<CacheValue>>;
+ static std::mutex mutex;
+ static std::map<LoadKey, std::shared_future<CacheResult>> pending;
Review Comment:
A process-wide static registry for coalescing cold loads seems out of place
here. In-flight dedup belongs to the cache (`FileBlockCache` keeps its pending
futures as instance state; `GenericLruCache::Get` deliberately loads outside
the lock), and it only helps the first concurrent reads of a manifest — after
that they are cache hits anyway. Suggest dropping it together with its test; if
it turns out to matter, add it once in the cache's `Get` so every cache kind
benefits.
##########
include/paimon/table/source/scan_metrics.h:
##########
@@ -25,6 +25,16 @@ namespace paimon {
/// Metric names for scan planning operations.
class PAIMON_EXPORT ScanMetrics {
public:
+ // Cumulative work for row-range manifest reads; duration histogram
samples are per file (ms).
+ static constexpr char ROW_RANGE_MANIFEST_ENTRIES_SCANNED[] =
"rowRangeManifestEntriesScanned";
Review Comment:
Seven new public metric names plus a per-file histogram (which the docs have
to warn must not be summed) is a lot for this change. The row-range
scanned/materialized counters overlap with the existing
`lastLazyDecodeScannedRows` / `lastLazyDecodeMaterializedRows`, and the
`read_metrics_` collected by `ManifestList` / `IndexManifestFile` is never
exported — only `manifest_file_->GetReadMetrics()` is merged in
`GetScanMetrics()`. Suggest keeping at most the manifest cache hit/miss
counters and dropping the rest, which also removes most of the new doc section.
--
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]
