-
Notifications
You must be signed in to change notification settings - Fork 202
Add LazyNotFound Store Optimization, Support for fast_slow_store (S3, GCS slow_store targets) #2072
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
MarcusSorealheis
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
would like to see tests
MarcusSorealheis
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry for the delay here. I'd been squeezing in work the past few days.
Overall, this looks good. I do have concerns that I have mentioned. I know the original issue may have used LazyNotFound but can we be even more explicit in referring to it.
I'm thinking LazyExistenceOnWrite or something that explains what exactly we are lazing about. Let me know your thoughts.
| NoopDownloads, | ||
|
|
||
| /// If the store will rely on lazily determining file existence for mutations. | ||
| LazyNotFound, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: I wonder about LazyNotFound but curious on your thoughts.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that your point is true. Building on your thought, I think the intent is only half-captured via the current name since what we are doing foregoes finding out anything about the file's existence until we attempt to start reading from it in a fast to slow store streamed sync.
I like where you're going with LazyExistenceOnWrite, but the actual optimization happens on reads, and more pedantically, outgoing streams triggered by a caller. Should we go with LazyExistenceOnSync? Or maybe SpeculativeRead. I'm good with any of these including yours, or something else if there are other ideas.
|
this is overall an awesome PR! |
Description
As requested, this implements an optimization that skips
.has()checks on streamed syncs from fast to supported slow stores (S3Store or GCSStore for now) inFastSlowStore.Fixes #1999
Type of change
not work as expected)
How Has This Been Tested?
Some light testing locally. But feedback is requested. I was thinking of writing a new mock/set of tests in
nativelink-store/tests/fast_slow_store_test.rs. Let me know if that sounds ok or if there is a better place.I don't believe a documentation change is needed for this optimization, but correct me if you disagree.
Checklist
bazel test //...passes locallygit amendsee some docsThis change is