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

Eagerly fetch stdout on remote execution response #5712

Merged
merged 6 commits into from Apr 19, 2018

Conversation

Projects
2 participants
@ity
Copy link
Member

ity commented Apr 16, 2018

Problem (#5511 )

Right now when remote execution finishes, we just return whether it was successful, and any inline stdout/stderr.

Instead, we should fetch the stdout and populate these fields

Solution

  • stdout now is populated using stdout_digest or stdout_raw
  • convert response to using futures

@ity ity added the remoting label Apr 16, 2018

@ity ity self-assigned this Apr 16, 2018

@ity ity requested a review from illicitonion Apr 16, 2018

@illicitonion
Copy link
Contributor

illicitonion left a comment

Looks great, thanks! All pretty minor clean-up comments, except: Can you extract a function and also do the same for stderr?

@@ -30,6 +31,29 @@ enum ExecutionError {
NotFinished(String),
}

macro_rules! futurify {

This comment has been minimized.

@illicitonion

illicitonion Apr 17, 2018

Contributor

I think you should be able to just use the implementation of futurify_with_ok_return for both of these macros. Maybe delete this one, rename futurify_with_ok_return to futurify, and just use that?

(Also, maybe call this try_future! to match the try! macro which exists in the standard library?)

let grpc_result = map_grpc_result(
operations_client.get_operation(&operation_request)
);
match grpc_result {

This comment has been minimized.

@illicitonion

illicitonion Apr 17, 2018

Contributor

This whole match can probably become future::ok(future::Loop::Continue(futurify!(operation))).to_boxed() as BoxFuture<_, _>

let stdout =
if execute_response.get_result().has_stdout_digest() {
self.store.load_file_bytes_with(
execute_response.get_result().get_stdout_digest().into(), |v: Bytes| v)

This comment has been minimized.

@illicitonion

illicitonion Apr 17, 2018

Contributor

The : Bytes probably isn't necessary here

self.store.load_file_bytes_with(
execute_response.get_result().get_stdout_digest().into(), |v: Bytes| v)
.map_err(|error| ExecutionError::Fatal(format!("Error fetching stdout: {:?}", error)))
.and_then(|maybe_value| match maybe_value {

This comment has been minimized.

@illicitonion

illicitonion Apr 17, 2018

Contributor

This .and_then(match) would probably be a little more concise as:
.ok_or_else(|| ExecutionError::Fatal("Couldn't find stdout when fetching".to_owned()))

This comment has been minimized.

@ity

ity Apr 18, 2018

Member

couldnt get this to work, maybe I need to state at it a little longer..

if execute_response.get_result().has_stdout_digest() {
self.store.load_file_bytes_with(
execute_response.get_result().get_stdout_digest().into(), |v: Bytes| v)
.map_err(|error| ExecutionError::Fatal(format!("Error fetching stdout: {:?}", error)))

This comment has been minimized.

@illicitonion

illicitonion Apr 17, 2018

Contributor

Maybe mention the digest which couldn't be fetched (and below)

use std::collections::BTreeMap;
use std::iter::{self, FromIterator};
use std::sync::Arc;
use std::time::Duration;

#[derive(Debug, PartialEq)]
enum StdoutType {
//

This comment has been minimized.

@illicitonion

illicitonion Apr 17, 2018

Contributor

Drop empty comments

stderr: as_byte_owned_vec(""),
exit_code: 0,
}
);
}

#[test]
fn successful_execution_after_one_getoperation_with_digest() {

This comment has been minimized.

@illicitonion

illicitonion Apr 17, 2018

Contributor

This test would probably be a little simpler as one which uses extract_execute_response rather than run_remote_command; I think it would look something like:

#[test]
fn extract_response_with_digest_stdout() {
  assert_eq(
    extract_execute_response(make_successful_operation("gimme-foo".to_owned(), StdoutType::Digest(digest()), "", 0),
    Ok(ExecuteProcessResult {
      stdout: str_bytes(),
      stderr: as_byte_owned_vec(""),
      exit_code: 0,
    })
  );
}

(And you'd need to change which function pre-populates the StubCAS)

This comment has been minimized.

@ity

ity Apr 17, 2018

Member

much better!

.gitignore Outdated
@@ -51,3 +51,4 @@ GRTAGS
GSYMS
GTAGS
.mypy_cache/
target

This comment has been minimized.

@illicitonion

illicitonion Apr 17, 2018

Contributor

Isn't this already covered by the .gitignore in src/rust/engine?

This comment has been minimized.

@ity

ity Apr 17, 2018

Member

odd this changed

@ity

This comment has been minimized.

Copy link
Member

ity commented Apr 18, 2018

sounds great re:stderr - ok if I follow up with another PR for it? or would you rather have it as a part of this one?

@ity ity force-pushed the ity:ity/5511 branch from cedab12 to d5d5f1b Apr 18, 2018

@illicitonion
Copy link
Contributor

illicitonion left a comment

LGTM - couple of trivial things to fix to make CI pass and we're good to merge :)

ity added some commits Apr 18, 2018

@illicitonion illicitonion merged commit b8d97a1 into pantsbuild:master Apr 19, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@illicitonion illicitonion added this to Done in Remoting via automation Apr 19, 2018

@ity ity deleted the ity:ity/5511 branch Apr 22, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment