Core: Skip length check in ContentCache.tryCache for cached locations#16787
Draft
rahulsmahadev wants to merge 1 commit into
Draft
Core: Skip length check in ContentCache.tryCache for cached locations#16787rahulsmahadev wants to merge 1 commit into
rahulsmahadev wants to merge 1 commit into
Conversation
Co-authored-by: Isaac
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Follow-up to #16762, recommended by @szehon-ho in his review there.
Unlike manifests, manifest-list files do not track their length in table metadata, so the
InputFilehanded toContentCache.tryCacheresolves its length with a remotegetFileStatus/HEAD call. SincetryCachegates oninput.getLength() <= maxContentLengthbefore wrapping, everyBaseSnapshot.cacheManifestscall pays that round-trip — including cache hits, which is exactly the table-refresh/streaming-poll case that manifest-list caching targets.This change probes the cache for the location first and only consults
getLength()for uncached locations, making cache hits round-trip-free:cache.asMap().containsKey(...)rather thangetIfPresent(...)so it does not record cache statistics — agetIfPresentprobe would count a hit before any read happens and skew stats consumers.maxContentLengthgate, so skipping the length check for cached locations does not change which files are cacheable.CachingInputFilefalls back to loading through the regular path, same as today.The change is independent of #16762 (
tryCacheis used for manifest caching on main today, where lengths are known locally and the gate is free), but it matters once manifest lists flow through the cache.Test plan
New
TestContentCachecovering:getLength()on the input (the test input throws if its length is resolved), and reads are served from the cacheAlso ran
TestManifestCaching(cache stats/size expectations unchanged) plus spotless locally.This pull request and its description were written by Isaac, an AI coding assistant (Claude Code), on behalf of @rahulsmahadev.