https://pantsbuild.org/ logo
Join Slack
Powered by
so in order to fully remove the `Bytes` `stderr` a...
# development
h
so in order to fully remove the 
Bytes
stderr
and 
stdout
fields from 
FallibleProcessResultWithPlatform
, I think we need to change the flow on the 
store
method in 
<http://cache.rs|cache.rs>
right now one of the things 
store()
does is read the bytes from the 
FallbileProcessResultWithPlatform
, create new digests by storing them in 
self.file_store
, and then puting those digests into 
self.process_execution_store
w
not quite, i don’t think
the process_execution_store holds a protobuf struct
which has digest fields
h
right
w
… so i think that in the new flow you just pass them in.
the per-file digests do not go in the process_execution_store.
just the protobuf, afaik.
but
does is read the bytes from the 
FallbileProcessResultWithPlatform
, create new digests by storing them in 
self.file_store
part of the reason this might be misleading is that these aren’t “new” digests
they’ll always be identical for the same bytes. so there is no new/old
and so, if you already have the digest, you’re good to go.
h
the thing I'm confused about is that, digests only make sense with respect to a specific store, right?
w
no
@hundreds-breakfast-49010: ish.
h
like, if you have a digest that was saved in one store, and try to read it from a different store, it will fail
w
but what the process_execution_store stores is only a protobuf
not the files or directories
h
so that means that the 
hasing::Digest
type is being stored as just the 40 hex characters within the protobuf, right?
w
the protobuf has a type for a digest
it’s the same protobuf as is used in remote execution
h
hm, and when we deserialize a 
FallibleProcessResultWithPlatform
in 
lookup()
, we'll just get back a 
Digest
that points at actual data stored in the store in the underlying CommandRunner, right?
so actually, we shouldn't even need 
self.file_store
anymore?
w
um, let me confirm some of what i’m saying. i expect that the process_execution_store is a cache from process request to response
…yea.
it’s a cache from the digest of a Request to an actual Response object (which was stored as protobuf)
but in the protocol, neither Request nor Response actually “contain” files/directories… instead, they have digests for them
and those file/directory digests are stored in our capital S Store
so: process_execution_store is only a cache from request to response, and the capital S Store is only a store of files/directories
@hundreds-breakfast-49010: does that make sense?
h
right. 
process_execution_store
is 
ShardedLmdb
, 
file_store
is 
Store
so what I'm saying is, I don't think there's any reason for 
file_store
to exist anymore
w
there is.
…oh. i see what you mean.
h
unless there's a way that the 
Store
that contains the data that a given 
Digest
contains can be lost somehow
w
because
Copy code
crate::remote::populate_fallible_execution_result(
        self.file_store.clone(),
        execute_response,
        vec![],
        platform,
      )
…doesn’t need to use the file store to load the content anymore?
yea, that makes sense.
@hundreds-breakfast-49010: ahm, re: https://pantsbuild.slack.com/archives/C0D7TNJHL/p1591218466108400?thread_ts=1591217721.101200&amp;cid=C0D7TNJHL … yes, that is still sortof a thing. although a change i’m about to make will make it much less likely
h
I think so? i.e. my change would entail removing 
store: Store
as an argument from 
populate_fallible_execution_result
but yeah, I think the data structurs that we have kind of divorce a 
Digest
from the 
Store
that actually contains the data that 
Digest
fingerprints
and at some point if we're passing around a 
Digest
we'll need to get the 
Bytes
for it out of a 
Store
, and if it's the wrong 
Store
that operation will just fail
w
@hundreds-breakfast-49010: yes to digests being used for multiple things.
but sortof no: the process_execution_store is not 
digest->content
… it’s 
digest->content_of_a_response
anyway… the 
<http://cache.rs|cache.rs>
file should be almost the only thing using digests for processes.
@hundreds-breakfast-49010: so, yes: i think you can remove the file_store from 
<http://cache.rs|cache.rs>
.
h
ok that makes sense to me
and that means that 
populate_remote_execution_result
also needs to not have a 
store
argument anymore
tom's recent commit calls that function in 
<http://streaming.rs|streaming.rs>
w
yea. all of this will move to somewhere where we lift the result for python.
…so, @hundreds-breakfast-49010: can i take back what i said above about removing the store from those codepaths?
h
yeah
espeically if this is about the way that 
<http://remote.rs|remote.rs>
uses 
extract_output_files
🙂
w
not because it’s necessary now (i don’t think it is), but because the change i’m making is going to use it.
h
hm, what change?
w
basically, when we hit the cache, we have a 
FalliablePRWP
things can be garbage collected out of the store. and to avoid that, we hold “leases” on files
h
that's exactly the class of error I was concerned about
w
right.
i don’t think that you have to worry about it, but in my change i’m going to add lease extension when we hit that cache
so if you could leave it (marked unused, if need be) that would be appreciated.
h
okay, so the last place we need to think about 
Store
concerns is in 
<http://intrinsics.rs|intrinsics.rs>
in 
multi_platform_process_request_to_process_results
oh wait, that has a 
Core
and 
Core
has a 
Store
. so maybe that's fine
w
ya
h
assuming that's the right store
w
there is only one.
(capital S Store)
Open in Slack
PreviousNext
Powered by