-
Notifications
You must be signed in to change notification settings - Fork 236
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
buck2_execute: implement OSS upload_blob
for local_only
cache uploads
#765
base: main
Are you sure you want to change the base?
buck2_execute: implement OSS upload_blob
for local_only
cache uploads
#765
Conversation
0088a2f
to
1aa195d
Compare
3b43d03
to
f70454a
Compare
f70454a
to
29e7e13
Compare
52dac90
to
685d6ec
Compare
685d6ec
to
8ff38d0
Compare
8ff38d0
to
9a28eff
Compare
030fbc0
to
38881c5
Compare
38881c5
to
d129ca7
Compare
@facebook-github-bot has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
6e1e32e
to
4599acf
Compare
9a92e6b
to
76f13b5
Compare
…oads Forward-port of patch 4 in <facebook#477>, providing a clear piece of missing functionality: in the event that stdout or stderr were more than 50KiB of output when caching `local_only` actions, then this dead path was taken, and so stdout/stderr would not be uploaded successfully in the cache. Co-authored-by: Austin Seipp <[email protected]>
76f13b5
to
f0bc22e
Compare
@@ -762,11 +762,21 @@ impl REClient { | |||
|
|||
pub async fn upload_blob( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
make it match internal RE signature:
pub async fn upload_blob( | |
pub async fn upload_blob_with_digest( | |
&self, | |
blob: Vec<u8>, | |
digest: TDigest, | |
metadata: RemoteExecutionMetadata, | |
) -> anyhow::Result<TDigest> { | |
let blob = InlinedBlobWithDigest{digest:digest.clone(), blob, ..Default::default()}; | |
self.upload( | |
metadata, | |
UploadRequest { | |
inlined_blobs_with_digest: Some(vec![blob]), | |
files_with_digest: None, | |
directories: None, | |
upload_only_missing: false, | |
..Default::default() | |
}, | |
) | |
.await?; | |
Ok(digest) | |
} |
@@ -1230,17 +1230,19 @@ impl RemoteExecutionClientImpl { | |||
|
|||
pub async fn upload_blob( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
updated to match signature used for internal RE call
pub async fn upload_blob( | |
pub async fn upload_blob( | |
&self, | |
blob: InlinedBlobWithDigest, | |
use_case: RemoteExecutorUseCase, | |
) -> buck2_error::Result<TDigest> { | |
with_error_handler( | |
"upload_blob", | |
self.get_session_id(), | |
self.client() | |
.upload_blob_with_digest(blob.blob, blob.digest, use_case.metadata(None)) | |
.await, | |
) | |
.await | |
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@thoughtpolice, commented with some small changes to make the PR apply cleanly vs the internal RE implementation, please apply, rebase and re-push the PR.
Forward-port of patch 4 in #477, providing a clear piece of missing functionality: in the event that stdout or stderr were more than 50KiB of output when caching
local_only
actions, then this dead path was taken, and so stdout/stderr would not be uploaded successfully in the cache.