Require no post-download validation for remote->FSDB #19815
Merged
Conversation
This file contains 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
thejcannon
approved these changes
Sep 11, 2023
Comment on lines
+281
to
+282
| // this test is ensuring that "huge" directories don't fall into the FSDB code paths, so let's | ||
| // ensure we're actually testing that, by validating that a _file_ of this size would use FSDB |
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.
This adjusts
RemoteStore::download_digest_to_localto enforce that thef_remotevalidation function is never provided for digests that should be stored in FSDB. This is no-functionality-change, just refactoring things to be slightly clearer and more future-proof.Previously, this code was conditionalising whether something should be stored in FSDB on more than just the type/size, which could have led to bugs: if we were downloading a digest that should be stored in FSDB, but pass
f_remote = Some(...),download_digest_to_localwould instead store it in LMDB, and future look-ups of the digest (which don't know aboutf_remote) would look in FSDB and fail to find it.However, in practice, we only ever pass
f_remoteinStore::load_directory, and directories are never stored in the FSDB, even if they're huge, so the extra condition was not changing behaviour. This PR is making the assumption/behaviour/luck here more explicit, and adds a test validating the behaviour of huge directory digests too.This came out of my investigation of #19748, and, I think, eliminates directories as potential cause of the issues there.