Upgrade to v2 of bazel protobuf #6027
Conversation
| @@ -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 | |||
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
Reworded both comments.
| grpcio::RpcStatusCode::Unimplemented, | ||
| None, | ||
| )); | ||
| unimplemented!() |
There was a problem hiding this comment.
Because this method is deprecated, or...?
| stream | ||
| .take(1) | ||
| .into_future() | ||
| // Drop the _stream to disconnect so that the server doesn't keep the connection alive and |
There was a problem hiding this comment.
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.
| )) | ||
| }) | ||
| }) | ||
| // The unwrap() below is safe because we have joined any futures that had references to the Arc |
There was a problem hiding this comment.
This comment is no longer relevant.
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
force-pushed
the
dwagnerhall/bazelv2
branch
from
1acf9a7 to
596538d
Compare
Key differences:
stream.
inlined.