Skip to content

Commit

Permalink
Use a trait for remote ByteStore network implementation (#19050)
Browse files Browse the repository at this point in the history
This separates the `store::remote::ByteStore` coordination code from the
gRPC REAPI implementation code, by:

- adding a `store::remote::ByteStoreProvider` trait
- moving the REAPI implementation into `store::remote::reapi` and
implementing that trait

This is, in theory, just a lift-and-shift, with no functionality change,
other than maybe changing some logging and when exactly a work-unit is
created.

This is partial preparation work for supporting more remote stores like
GHA cache or S3, for #11149, and is specifically broken out of #17840.
Additional work required to actually solve #11149:

- generalising the action cache too
- implementing other byte store providers
- dynamically choosing the right provider for `store::remote::ByteStore`

The commits are individually reviewable:

1. preparatory clean-up
2. define the trait
3. move the REAPI code and implement the trait, close to naively as
possible:
  - https://gist.github.com/huonw/475e4f0c281fe39e3d0c2e7e492cf823 has a
white-space-ignoring diff between `remote.rs` after 1, and the
`reapi.rs` file in this commit
  - there are some changes: making functions non-generic, and eliminating
the work-units etc. (all the code that was left in `remote.rs`)
5. minor clean-up
6. more major clean-up: inline the method calls into the trait
definition, so that they're not just `fn method(&self, ...) {
self.real_method(...) }`:
  - as with 3, this is just a move, without changing the details of the
code...
  - ...except for some minor changes like converting `?` into `.map(|e|
e.to_string())?` on the `get_capabilities` call in `store_bytes`
  • Loading branch information
huonw authored May 23, 2023
1 parent 4e9c824 commit cc8be36
Show file tree
Hide file tree
Showing 4 changed files with 432 additions and 377 deletions.
9 changes: 2 additions & 7 deletions src/rust/engine/fs/store/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -801,9 +801,7 @@ impl Store {
ingested_digests.keys().cloned().collect()
} else {
remote
.list_missing_digests(
remote.find_missing_blobs_request(ingested_digests.keys().cloned()),
)
.list_missing_digests(ingested_digests.keys().cloned())
.await?
};

Expand Down Expand Up @@ -958,10 +956,7 @@ impl Store {
} else {
return Ok(false);
};
let missing = remote
.store
.list_missing_digests(remote.store.find_missing_blobs_request(missing_locally))
.await?;
let missing = remote.store.list_missing_digests(missing_locally).await?;

Ok(missing.is_empty())
}
Expand Down
Loading

0 comments on commit cc8be36

Please sign in to comment.