https://pantsbuild.org/ logo
#development
Title
# development
h

hundreds-breakfast-49010

06/03/2020, 8:55 PM
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

witty-crayon-22786

06/03/2020, 8:56 PM
not quite, i don’t think
the process_execution_store holds a protobuf struct
which has digest fields
h

hundreds-breakfast-49010

06/03/2020, 8:57 PM
right
w

witty-crayon-22786

06/03/2020, 8:57 PM
… 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

hundreds-breakfast-49010

06/03/2020, 8:59 PM
the thing I'm confused about is that, digests only make sense with respect to a specific store, right?
w

witty-crayon-22786

06/03/2020, 9:00 PM
no
@hundreds-breakfast-49010: ish.
h

hundreds-breakfast-49010

06/03/2020, 9:00 PM
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

witty-crayon-22786

06/03/2020, 9:00 PM
but what the process_execution_store stores is only a protobuf
not the files or directories
h

hundreds-breakfast-49010

06/03/2020, 9:00 PM
so that means that the
hasing::Digest
type is being stored as just the 40 hex characters within the protobuf, right?
w

witty-crayon-22786

06/03/2020, 9:01 PM
the protobuf has a type for a digest
it’s the same protobuf as is used in remote execution
h

hundreds-breakfast-49010

06/03/2020, 9:02 PM
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

witty-crayon-22786

06/03/2020, 9:03 PM
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

hundreds-breakfast-49010

06/03/2020, 9:07 PM
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

witty-crayon-22786

06/03/2020, 9:07 PM
there is.
…oh. i see what you mean.
h

hundreds-breakfast-49010

06/03/2020, 9:07 PM
unless there's a way that the
Store
that contains the data that a given
Digest
contains can be lost somehow
w

witty-crayon-22786

06/03/2020, 9:08 PM
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

hundreds-breakfast-49010

06/03/2020, 9:09 PM
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

witty-crayon-22786

06/03/2020, 9:10 PM
@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

hundreds-breakfast-49010

06/03/2020, 9:12 PM
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

witty-crayon-22786

06/03/2020, 9:13 PM
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

hundreds-breakfast-49010

06/03/2020, 9:35 PM
yeah
espeically if this is about the way that
<http://remote.rs|remote.rs>
uses
extract_output_files
🙂
w

witty-crayon-22786

06/03/2020, 9:35 PM
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

hundreds-breakfast-49010

06/03/2020, 9:36 PM
hm, what change?
w

witty-crayon-22786

06/03/2020, 9:36 PM
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

hundreds-breakfast-49010

06/03/2020, 9:36 PM
that's exactly the class of error I was concerned about
w

witty-crayon-22786

06/03/2020, 9:36 PM
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

hundreds-breakfast-49010

06/03/2020, 10:38 PM
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

witty-crayon-22786

06/03/2020, 10:39 PM
ya
h

hundreds-breakfast-49010

06/03/2020, 10:39 PM
assuming that's the right store
w

witty-crayon-22786

06/03/2020, 10:39 PM
there is only one.
(capital S Store)