Skip to content
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

Implement lease extension #17944

Closed
wants to merge 3 commits into from
Closed

Conversation

coeuvre
Copy link
Member

@coeuvre coeuvre commented Mar 31, 2023

Add flag --experimental_remote_cache_lease_extension, which when set, Bazel will create a background thread periodically sending FindMissingBlobs requests to CAS during the build.

  1. All the outputs that were not downloaded are within the scope of lease extension. The outputs are acquired from skyframe by traversing the action graph.
  2. Lease extension starts after any action was built and ends after execution phase ended. The frequency is related to --experimental_remote_cache_ttl.
  3. Lease extensions are performed on action basis, not by collecting all outputs and issue one giant FindMissingBlobs.
    • Collecting all outputs might increase memory watermark and cause OOM.
    • Sending one FindMissingBlobs request per action may increase the overhead of network roundtrip, but the cost should be saturated given that the lease extension happens at background and is not wall time critical.
  4. For an incremental build, the same applies: lease extension starts after any action was executed.
    • We don't want lease extension blocking action execution, nor affecting build performance.
    • Since we have TTL based cache discarding, any expired blobs will be discarded.
    • Leases of blobs that are not downloaded, still used by this build (because they are referenced by skyframe) will be extended as normal.

Part of #16660.

TODO: Sending out now for early feedbacks. Unit and integration tests will be added later.

@brentleyjones
Copy link
Contributor

Just a question, but why was 9c96529 done before this was merged?

@coeuvre
Copy link
Member Author

coeuvre commented Jun 27, 2023

I don't think is this a blocker for 9c96529. but this is now under review and should be merged soon anyway.

@brentleyjones
Copy link
Contributor

I don't think is this a blocker for 9c96529

I disagree 😅, but I'm glad it's being merged soon! 😄

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants