Store can backfill from a CAS #5148

illicitonion · 2017-12-01T16:46:03Z

This uses the ContentAddressableStorageService protocol specified in
https://github.com/googleapis/googleapis/blob/master/google/devtools/remoteexecution/v1test/remote_execution.proto

There are a few of problems with this implementation:

It has one unnecessary synchronisation point inside
load_bytes_remote_with - I'm going to work this out before merging.
It doesn't currently pass around the sizes of Digests. I have an
ongoing discussion with both the Bazel API specifiers, and the scoot
team, about what we should do here. Passing around a size, or a size
hint, is easy to do, but I don't want to commit to how it's going to
be done until we know whether it needs to be.
It is hard-coded to run with a one-thread threadpool. I'm not sure
whether we should be configuring this statically (like we do for the
IO pool), or through from a pants config. If the latter, I haven't
worked out how to do that yet.
Context doesn't currently accept a CAS address (specified by pants
config). I will wire this up in a subsequent PR.

This uses the ContentAddressableStorageService protocol specified in https://github.com/googleapis/googleapis/blob/master/google/devtools/remoteexecution/v1test/remote_execution.proto There are a few of problems with this implementation: 1. It has one unnecessary synchronisation point inside load_bytes_remote_with - I'm going to work this out before merging. 2. It doesn't currently pass around the sizes of Digests. I have an ongoing discussion with both the Bazel API specifiers, and the scoot team, about what we should do here. Passing around a size, or a size hint, is easy to do, but I don't want to commit to how it's going to be done until we know whether it needs to be. 3. It is hard-coded to run with a one-thread threadpool. I'm not sure whether we should be configuring this statically (like we do for the IO pool), or through from a pants config. If the latter, I haven't worked out how to do that yet. 4. Context doesn't currently accept a CAS address (specified by pants config). I will wire this up in a subsequent PR.

stuhood

(more feedback to follow)

stuhood · 2017-12-01T18:17:33Z

src/rust/engine/fs/src/store.rs

+  grpc_env: Option<GrpcEnvironment>,
+}
+
+#[derive(Clone)]


I don't see this implementation of Clone used: should remove if possible. InnerStore is not Clone, so avoiding sending the signal that this needs to be clone would be good.

Clone happens here: a01221f#diff-5f54ce2d41ce4be118aab4eb95da8fe1R186

Could that closure just reference store.grpc_env? You've sent the store clone into the closure already...

stuhood · 2017-12-01T18:22:54Z

src/rust/engine/fs/src/store.rs

+    let grpc_env = self.inner.grpc_env.clone();
+    let store = self.clone();
+
+    let f = Arc::new(Mutex::new(f));


I think that if you add an F: Sync* bound (which should be easy enough: the contents of the closure are generally file paths, etc), you wouldn't need the Mutex here, and could just use an Arc.

But also, I think that my factoring of load_bytes_with might have been a bit premature: it exists for two reasons: 1) to avoid a copy, 2) to allow the file-io of dumping into a file to run directly on the pool. If it simplifies things to have an extra copy from gRPC to the store (ie load_bytes), and then only use the *_with for the copy out of the store, that's probably totally fine too.

* maybe also +Send? (I haven't fully internalized when you need each of them)

It's already +Send, and indeed making it +Sync works :)

stuhood · 2017-12-01T18:53:47Z

src/rust/engine/fs/src/store.rs

+
+    // TODO: Work out why not wrapping this in a future::done(... .wait()) errors in gRPC channel
+    // creation.
+    future::done(stream.map(|r| r.data).concat2().wait())


Hm. That is certainly suspicious.

You're saying that this API already exposes Futures, but that if you don't call wait, you see an error? That would definitely imply a bug in the library, because wait should never actually be necessary except at the root of a Future hierarchy.

Filed tikv/grpc-rs#123 and added a workaround to prevent client from dropping early.

stuhood · 2017-12-01T18:55:13Z

src/rust/engine/fs/src/store.rs

+          }
+        }
+      })
+      .to_boxed()


This first to_boxed call should not be necessary, as flatten probably doesn't have a Sized bound.

stuhood · 2017-12-01T18:55:42Z

src/rust/engine/fs/src/store.rs

+        }
+      })
+      .to_boxed()
+      .flatten()


Rather than flatten, you can change your map to an and_then (which flattens).

stuhood · 2017-12-01T18:58:55Z

src/rust/engine/fs/src/store.rs

@@ -175,6 +234,83 @@ impl Store {
    })
  }

+  fn load_bytes_remote_with<T: Send + 'static, F: Fn(&[u8]) -> T + Send + 'static>(


I feel funny about both local and remote logic existing in the same impl.

I'm not sure whether having a Store trait with "local", "remote", "cached" implementations would be maximally efficient, but I do not know that it results in a very clean factoring. Will send you a link to an internal thing.

Sure, I'll pull them out into separate impls :)

stuhood · 2017-12-01T19:00:29Z

src/rust/engine/fs/src/store.rs

+      .and_then(move |maybe_bytes: Option<Vec<u8>>| match maybe_bytes {
+        Some(bytes) => {
+          if db == directory_db {
+            match Store::validate_directory(&bytes) {


I think this can be:

if db == directory_db { Store::validate_directory(&bytes).map_err(..)?; }

... if the return statement is valid, then the ? should be as well.

Alas no; because the return type of this closure is a BoxFuture (chained off of the store_copy.store_bytes), it isn't considered to return a Result, so can't be ?'d.

stuhood

Thanks @illicitonion : this looks great.

The only blocker would be that wait I think... if we leave it in, we should try to execute it directly at the root of a CpuPool, as calling wait in a Future context has been deadlock prone in the other Future impls I've used, and I expect that it's the same here.

stuhood · 2017-12-01T20:30:57Z

src/rust/engine/fs/src/store.rs

+                    }
+                  }).map(move |()| {
+                let f = f.lock().unwrap();
+                Some(f(&bytes))


I think that if you ran the user function before the store_bytes, you wouldn't need to clone the bytes in order to store them?

I think that that altered ordering would be harmless, as we never guaranteed that the load_bytes_into was atomic.

This feels a little weird, but I guess since I made it a Fn rather than a FnOnce... Done.

stuhood · 2017-12-01T20:36:06Z

src/rust/engine/fs/src/store.rs

+                Some(f(&bytes))
+              }).to_boxed()
+        }
+        None => future::ok(None).to_boxed() as BoxFuture<_, _>,


are the as BoxFuture ascriptions actually necessary...?

Sadly so. Otherwise I'm told:

= note: expected type `std::boxed::Box<futures::Map<futures::AndThen<futures_cpupool::CpuFuture<hash::Fingerprint, std::string::String>, std::result::Result<(), std::string::String>, [closure@src/store.rs:280:29: 291:20 fingerprint:_]>, [closure@src/store.rs:291:26: 291:42 result:_]>>` found type `std::boxed::Box<futures::FutureResult<std::option::Option<_>, _>>` note: match arm with an incompatible type

This is rust-lang/rust#46061

illicitonion · 2017-12-04T18:07:51Z

Dropped in favour of #5154 and #5155 and one more coming soon

illicitonion requested a review from stuhood December 1, 2017 16:46

Extract StubCAS constructors

7b13494

illicitonion force-pushed the dwagnerhall/fs/cas-1 branch from 8368dc6 to 7b13494 Compare December 1, 2017 18:32

fs_util optionally uses a CAS server

e8c39c4

stuhood reviewed Dec 1, 2017

View reviewed changes

stuhood approved these changes Dec 1, 2017

View reviewed changes

illicitonion added 5 commits December 4, 2017 09:19

Keep client alive for its channel

0288d1d

Make F Sync, remove Mutex

b4e142b

and_then not map+flatten

2981274

Re-order f call to avoid clone

f056270

Remove a couple of unnecessary ascriptions

fc3b171

illicitonion closed this Dec 4, 2017

illicitonion deleted the dwagnerhall/fs/cas-1 branch February 26, 2018 11:28

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Store can backfill from a CAS #5148

Store can backfill from a CAS #5148

illicitonion commented Dec 1, 2017

stuhood left a comment

stuhood Dec 1, 2017

illicitonion Dec 4, 2017

stuhood Dec 4, 2017 •

edited

Loading

stuhood Dec 1, 2017

illicitonion Dec 4, 2017

stuhood Dec 1, 2017

illicitonion Dec 4, 2017

stuhood Dec 1, 2017

illicitonion Dec 4, 2017

stuhood Dec 1, 2017

illicitonion Dec 4, 2017

stuhood Dec 1, 2017

illicitonion Dec 4, 2017

stuhood Dec 1, 2017

illicitonion Dec 4, 2017

stuhood left a comment

stuhood Dec 1, 2017

illicitonion Dec 4, 2017

stuhood Dec 1, 2017

illicitonion Dec 4, 2017

illicitonion commented Dec 4, 2017

Store can backfill from a CAS #5148

Store can backfill from a CAS #5148

Conversation

illicitonion commented Dec 1, 2017

stuhood left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stuhood Dec 4, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stuhood left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

illicitonion commented Dec 4, 2017

stuhood Dec 4, 2017 •

edited

Loading