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

Upgrade to v2 of bazel protobuf #6027

Merged
merged 2 commits into from Jul 10, 2018

Conversation

Projects
None yet
3 participants
@illicitonion
Copy link
Contributor

illicitonion commented Jun 26, 2018

Key differences:

  • Execute is a streaming RPC - we only support a single response per
    stream.
  • Action is now a separate proto addressed by digest, rather than
    inlined.
  • Inline output files are no longer supported.
  • Output files are now specified on Command not Action.

@illicitonion illicitonion requested review from stuhood , dgassaway and dotordogh Jun 26, 2018

@stuhood
Copy link
Member

stuhood left a comment

Thanks!

@@ -39,6 +39,40 @@ enum ExecutionError {
NotFinished(String),
}

impl CommandRunner {
// The Execute API used to be unary, and became streaming. The contract of the streaming API is
// that if the client disconnects after one request, it should continue to function exactly like

This comment has been minimized.

@stuhood

stuhood Jun 27, 2018

Member

Is this a literal disconnection, ie closed connection? Or is this a single stream being closed on a multiplexed connection? Potentially expensive if the former...

Maybe worth a TODO to drop this when servers have migrated, if this is a compatibility thing.

This comment has been minimized.

@illicitonion

illicitonion Jul 10, 2018

Contributor

The underlying channel stays open, but the stream on top of it gets closed.

Reworded, added comment about possible future.

stream
.take(1)
.into_future()
// Drop the _stream to disconnect so that the server doesn't keep the connection alive and

This comment has been minimized.

@stuhood

stuhood Jun 27, 2018

Member

Comment moved below?

This comment has been minimized.

@illicitonion

illicitonion Jul 10, 2018

Contributor

Reworded both comments.

grpcio::RpcStatusCode::Unimplemented,
None,
));
unimplemented!()

This comment has been minimized.

@stuhood

stuhood Jun 27, 2018

Member

Because this method is deprecated, or...?

This comment has been minimized.

@illicitonion

illicitonion Jul 10, 2018

Contributor

Added comment

@dotordogh
Copy link
Contributor

dotordogh left a comment

Looks great! Some changes took some time to understand, but makes sense now! I'm exited to see this work end to end!

stream
.take(1)
.into_future()
// Drop the _stream to disconnect so that the server doesn't keep the connection alive and

This comment has been minimized.

@dotordogh

dotordogh Jun 27, 2018

Contributor

Maybe adding to the comment that this is dropping the stream if this is an error, and another comment for the other line that it is dropping the stream if it succeeds.

This comment has been minimized.

@illicitonion

illicitonion Jul 10, 2018

Contributor

Done

))
})
})
// The unwrap() below is safe because we have joined any futures that had references to the Arc

This comment has been minimized.

@dotordogh

dotordogh Jun 27, 2018

Contributor

This comment is no longer relevant.

This comment has been minimized.

@illicitonion

illicitonion Jul 10, 2018

Contributor

Removed

illicitonion added some commits Jun 25, 2018

Upgrade to v2 of bazel protobuf
Key differences:
 * Execute is a streaming RPC - we only support a single response per
   stream.
 * Action is now a separate proto addressed by digest, rather than
   inlined.
 * Inline output files are no longer supported.
 * Output files are now specified on Command not Action.

@illicitonion illicitonion force-pushed the twitter:dwagnerhall/bazelv2 branch from 1acf9a7 to 596538d Jul 10, 2018

@illicitonion illicitonion merged commit 1e6ae97 into pantsbuild:master Jul 10, 2018

1 check passed

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

@illicitonion illicitonion deleted the twitter:dwagnerhall/bazelv2 branch Jul 10, 2018

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