DeviousCardi commented on code in PR #20383:
URL: https://github.com/apache/hudi/pull/20383#discussion_r4304826812
##########
hudi-common/src/main/java/org/apache/hudi/core/read/BaseHoodieTableFileIndex.java:
##########
@@ -239,32 +297,51 @@ public List<PartitionPath> getAllQueryPartitionPaths() {
* Returns all listed file-slices w/in the partition paths returned by
{@link #getAllQueryPartitionPaths()}
*/
protected Map<PartitionPath, List<FileSlice>> getAllInputFileSlices() {
- if (!areAllFileSlicesCached()) {
- ensurePreloadedPartitions(getAllQueryPartitionPaths());
- }
-
- return cachedAllInputFileSlices;
+ // NOTE: We always hand out a freshly assembled snapshot rather than the
cache itself: the cache is mutated (and
+ // evicted from) by every subsequent query, so returning it would
alias the caller's result to live state.
+ return getInputFileSlices(getAllQueryPartitionPaths().toArray(new
PartitionPath[0]));
Review Comment:
Agreed. This limits cached file slices retained between queries; it does not
cap the peak memory of one query. I clarified that in the config documentation
and PR description in 4bd1972da1a. Queries still build the complete listing,
and the separate size ledger remains outside this budget.
##########
hudi-common/src/main/java/org/apache/hudi/core/read/BaseHoodieTableFileIndex.java:
##########
@@ -132,9 +149,50 @@ public abstract class BaseHoodieTableFileIndex implements
AutoCloseable {
private final transient FileStatusCache fileStatusCache;
// NOTE: Individual partitions are always cached in full: meaning that if
partition is cached
- // it will hold all the file-slices residing w/in the partition
+ // it will hold all the file-slices residing w/in the partition.
+ //
+ // Unless the spillable map is in use this is the map view of
[[fileSliceCache]], i.e. a Caffeine cache, and
+ // is therefore a genuinely thread-safe ConcurrentMap. In the
lazy-listing case its retention is additionally
+ // bounded by [[HOODIE_FILE_INDEX_MAX_MEMORY]], because there the
resident set is an accumulation of the
+ // working sets of every query this index has ever served and the
index itself can live for the whole session.
+ // In the eager-listing case the resident set is, by construction,
exactly the table's partitions as of the
+ // last refresh, so it does not grow with the session and is left
unbounded.
private transient volatile Map<PartitionPath, List<FileSlice>>
cachedAllInputFileSlices = new HashMap<>();
+ // NOTE: The Caffeine cache that [[cachedAllInputFileSlices]] is the
[[Cache#asMap]] view of. Nothing in production
+ // reads it -- all use goes through that view -- but the view
deliberately exposes no way back to the cache's
+ // own API, which is what the tests need in order to assert on the
eviction policy and its weighted size.
+ // Null until the first refresh, and stale -- i.e. still referring to
the previous Caffeine cache -- once a
+ // refresh has switched this index over to the spillable map, since
that branch replaces
+ // [[cachedAllInputFileSlices]] without touching this field. So it is
only meaningful alongside
+ // [[cachedAllInputFileSlices]], never on its own.
+ @VisibleForTesting
Review Comment:
Removed the annotation and its unused import in 4bd1972da1a; the field stays
private. All 21 focused tests pass, along with Checkstyle and license checks.
--
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]
