-
Notifications
You must be signed in to change notification settings - Fork 202
Fast slow store directions #1581
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
Fast slow store directions #1581
Conversation
f014616 to
69e7103
Compare
69e7103 to
194e7e4
Compare
194e7e4 to
251dda7
Compare
|
Hi @chrisstaite, not sure if I screwed anything up here yet, but I did just fix the merge conflicts to make it easier for @aaronmondal to review the PR and get it merged. It's a powerful new feature. I did this from the web browser so I may have screwed something up. It's clear that the PR needs:
There are a couple things missing in the PR that I'd like to see that are probably separate from what Aaron's going to ask about: One, an example of how to use it. A short guide would be awesome. #1577 makes it obvious why someone would want this feature (e.g., flow from Prod to Dev but not vice versa), but for other people to use it, would be great for us to document it. Aaron and I can also help there if time-constraints are an issue. We are simply trying to graduate Nativelink from the era of if users want to use it they need to look at the code, to opening up to more users. This is really a fantastic feature. Thanks for the contribution! |
|
i'll do the whole amend if my changes fixed the rust formatting issues and I think it wil. |
There are multiple use cases where we don't want a fast-slow store to persist to one of the stores in some direction. For example, worker nodes do not want to store build results on the local filesystem, just with the upstream CAS. Another case would be the re-use of prod action cache in a dev environment, but not vice-versa. This PR introduces options to the fast-slow store which default to the existing behaviour, but allows customisation of each side of the fast slow store to either persist in the case or get operations, put operations or to make them read only. Fixes TraceMachina#1577
c3c5436 to
bc6a438
Compare
|
@MarcusSorealheis I've brought this back up to date. |
|
Would be nice to get this in to the next release as it opens a lot of opportunity for cluster cost savings to avoid extra GCS operations. |
|
@chrisstaite I've just updated the branch. Will try it out. |
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 this is helpful. One question, though. If both directions are ReadOnly and a put arrives, it drains but returns Ok(())—is this correct? It discards data silently. Maybe log a warn! or error if unintended.
I don't want to fail totally silently.
|
@amankrx do you have any thoughts here? |
|
I'll test out the branch, but the CI is failing right now. Maybe, we should by investigating the cause here. |
|
@amankrx CI did not fail here did it? |
|
It failed after it was merged to main. Might not be related to this PR directly, but we should investigate why we get these errors: https://github.com/TraceMachina/nativelink/actions/runs/18865879330/job/53833330211 |
Description
There are multiple use cases where we don't want a fast-slow store to persist to one of the stores in some direction. For example, worker nodes do not want to store build results on the local filesystem, just with the upstream CAS. Another case would be the re-use of prod action cache in a dev environment, but not vice-versa.
This PR introduces options to the fast-slow store which default to the existing behaviour, but allows customisation of each side of the fast slow store to either persist in the case or get operations, put operations or to make them read only.
Fixes #1577
Type of change
Please delete options that aren't relevant.
How Has This Been Tested?
Added new tests
Checklist
bazel test //...passes locallygit amendsee some docsThis change is