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

Async (remote) cache writes #11434

Closed
stuhood opened this issue Jan 8, 2021 · 0 comments · Fixed by #11479
Closed

Async (remote) cache writes #11434

stuhood opened this issue Jan 8, 2021 · 0 comments · Fixed by #11479
Assignees

Comments

@stuhood
Copy link
Member

stuhood commented Jan 8, 2021

Currently the remote Cache writes synchronously to the cache when it receives a successful result. This write should be async.

Making the write async is trivial: adding a tokio::spawn that the runner does not wait for... i.e.:

let _cache_write = tokio::spawn(async move { /* ... write to the cache */ });

Testing the change, is slightly more challenging. Two potential options:

  1. Adding a delay to the StubActionCache for cache writes, and then demonstrating that CommandRunner::run takes less than that delay (and that the StubActionCache does still receive the write).
  2. Extracting a trait for the "Cache" implementation itself, and using a mock cache implementation in the test that you use to validate that the run method completes before the cache write.

The latter implementation is more general, and has some other benefits: with an extracted Cache implementation, we could use the CommandRunner implementation for both local and remote running, with both speculation and async writes.

@Eric-Arellano Eric-Arellano self-assigned this Jan 20, 2021
Eric-Arellano added a commit that referenced this issue Jan 22, 2021
Closes #11434. This should avoid writing to the cache from noticeably slowing down Pants's performance.

This does add a new risk that some cache writes will not happen, particularly when Pantsd is not in use. To assess the problem, we add new metrics for when a cache write starts and finishes. If it becomes a serious problem, we can revisit this, including possibly requiring that Pantsd be used for remote cache writes.
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 a pull request may close this issue.

2 participants