wangyong9999 commented on code in PR #329:
URL: https://github.com/apache/paimon-cpp/pull/329#discussion_r4022149185
##########
src/paimon/core/utils/snapshot_manager.cpp:
##########
@@ -124,7 +127,9 @@ bool SnapshotManager::ExpiredSinceBoundaryWasRead(int64_t
id) const {
}
Result<Snapshot> SnapshotManager::LoadSnapshot(int64_t snapshot_id) const {
- return Snapshot::FromPath(fs_, SnapshotPath(snapshot_id));
+ return snapshot_cache_.Get(snapshot_id, [this](const int64_t& id) ->
Result<Snapshot> {
Review Comment:
Caching snapshots by ID here applies to every `SnapshotManager`, not only
the ones held by `TableScanResources`, and several callers rely on
`LoadSnapshot` failing with `NotExist` to learn that a snapshot is gone.
`LatestSnapshotOfUserAtOrBefore` walks IDs downward and reads `NotExist` as
the end of history — or, when the latest came from the catalog, as the case
that must first pass `ExpiredSinceBoundaryWasRead`. A cache hit now makes an
expired snapshot look live and lets it be reported as that user's last commit,
which is exactly the outcome the comment there warns about ("could cause a
recovering writer to commit twice"). `FindSnapshotBeforeTimestamp`'s binary
search has the same dependency.
The cache also gets primed with the wrong entries: `ExpireSnapshots` loads
every snapshot it is about to delete through this same manager, so those IDs
are hot in the cache the moment their files disappear. The bypass this PR adds
in `ExpireSnapshots::ExpireUntil` ("Cached metadata cannot prove that the
current file has been published") is the same problem, noticed at one call site.
I would keep the cache out of `SnapshotManager` and put it where this PR's
scope is — behind `TableScanResources`, or on a separate accessor that
existence-sensitive callers do not go through.
##########
src/paimon/core/operation/scan_context.cpp:
##########
@@ -53,10 +56,19 @@ ScanContext::ScanContext(const std::string& path, bool
is_streaming_mode,
table_schema_(table_schema),
options_(options),
cache_(cache),
- format_table_(format_table) {}
+ format_table_(format_table),
+ table_resources_(table_resources) {}
ScanContext::~ScanContext() = default;
+std::shared_ptr<FileSystem> ScanContext::GetSpecificFileSystem() const {
Review Comment:
The four sibling context classes (`read_context.h`, `write_context.h`,
`commit_context.h`, `orphan_files_cleaner.h`) all keep this accessor as "the
file system the caller explicitly set". Folding the resources' file system in
changes what the name answers, and it disarms the check it feeds:
`TableScan::Create` passes `GetSpecificFileSystem()` into
`resources.Validate(...)`, so when the caller set none it compares the
resources' own file system against itself and the mismatch branch cannot fire
there. `Finish()` is then the only place that really checks, and it has already
rejected the conflicting case.
Keeping the accessor literal and resolving the effective file system at the
two or three call sites that need it would leave both the name and the check
meaning something.
--
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]
