[server] Add support of sharable file cache capacity for historical partition lookup - #4113
[server] Add support of sharable file cache capacity for historical partition lookup#4113zuston wants to merge 1 commit into
Conversation
fresh-borzoni
left a comment
fresh-borzoni
left a comment
There was a problem hiding this comment.
@zuston Thank you for the PR, left some questions and comments
#3955 also asks that the lookuper cache stop deciding when files get evicted. Paimon ties file lifetime to the query, so I don't think that's doable without the upstream withLookupFileCache change you mentioned.
Separately, this deletes LookuperContext while #4124 adds a field to it.
| .removalListener(this::onLookuperRemoved) | ||
| .build(); | ||
| this.lookuperIdSequence = new AtomicLong(); | ||
| this.lookupRuntime = createLookupRuntime(conf); |
There was a problem hiding this comment.
This builds the IOManager against the cache root, then startup() deletes and recreates that
directory. Only works because Paimon creates the spill dir lazily on first createChannel.
Mb move it into startup()?
| checkNotNull(fileRetention, "fileRetention must not be null."); | ||
| checkNotNull(maxDiskSize, "maxDiskSize must not be null."); | ||
| this.cache = | ||
| Caffeine.newBuilder() |
There was a problem hiding this comment.
#3955 asks for eviction metrics. capacityEvictions counts lookuper evictions, not file evictions, and
without recordStats() we can't count those. Shall we add it?
| sharedCache.updateMaxDiskSize(MemorySize.ofKibiBytes(1)); | ||
| secondNamespace.put("third-file", lookupFile(thirdFile)); | ||
| assertThat(Arrays.asList(secondFile, thirdFile).stream().filter(File::exists).count()) | ||
| .isLessThanOrEqualTo(1L); |
There was a problem hiding this comment.
The eviction here only ever happens inside secondNamespace, so it doesn't show one namespace
pushing another's files out which is the point of the PR. Worth adding, and asserting the count
rather than <= 1?
| // ponytail: one runtime-wide retention; add a server option if this needs tuning. | ||
| this.lookupFileCache = | ||
| new SharedLookupFileCache( | ||
| CoreOptions.LOOKUP_CACHE_FILE_RETENTION.defaultValue(), |
There was a problem hiding this comment.
Before, LocalTableQuery read lookup.cache-file-retention off the table's own options. Now it's
always the 1h default, this is the only reader of it left in the PR. Is it intentional?
|
|
||
| @Override | ||
| public void invalidateAll() { | ||
| // ponytail: O(n) namespace scan; add a namespace index if cache cardinality makes close |
There was a problem hiding this comment.
what is this ponytail, is it some kind of TODO?
Yes. We may also need to extend this PR to address the issue raised in #4158. I’ll work on it this week. |
2 participants
Purpose
Linked issue: close #3955
This PR depends on the #4050 .
Brief change log
As discussed in #3955, sharing the file cache across multiple table lookups requires maintaining a Fluss-side fork of Paimon’s LocalTableQuery, adapted to use the shared cache.
Tests
API and Format
Documentation