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

Handle RPC errors as well as message-inline errors #6589

Merged
merged 3 commits into from Oct 5, 2018

Conversation

Projects
None yet
2 participants
@illicitonion
Copy link
Contributor

illicitonion commented Oct 3, 2018

This required a minor change to grpcio
(pantsbuild/grpc-rs@4dfafe9)
to not throw away the information from the grpc stack.

illicitonion added some commits Oct 3, 2018

Handle RPC errors as well as message-inline errors
This required a minor change to grpcio
(pantsbuild/grpc-rs@4dfafe9)
to not throw away the information from the grpc stack.
Add ignored test
Doesn't work right now because servers can't fail with a Status

@illicitonion illicitonion requested review from stuhood , ity , blorente and dotordogh Oct 3, 2018

@stuhood

stuhood approved these changes Oct 4, 2018

@@ -61,15 +67,18 @@ impl CommandRunner {
stream
.take(1)
.into_future()
// If there was a response, drop the _stream to disconnect so that the server doesn't keep
// the connection alive and continue sending on it.
.map(|(maybe_operation, _stream)| maybe_operation)

This comment has been minimized.

@stuhood

stuhood Oct 4, 2018

Member

Worth explicitly dropping with mem::drop and placing the comment there? On the other hand, maybe doing it this way is less confusing for a new rust user because it doesn't make mem::drop seem magical.

This comment has been minimized.

@illicitonion

illicitonion Oct 5, 2018

Contributor

Done

@@ -1466,6 +1514,82 @@ mod tests {
}
}

//#[test] // TODO: Unignore this test when the server can actually fail with status protos.

This comment has been minimized.

@stuhood

stuhood Oct 4, 2018

Member

Worth creating a ticket for this?

This comment has been minimized.

@illicitonion

illicitonion Oct 5, 2018

Contributor

Done

@stuhood stuhood merged commit b1e73b5 into pantsbuild:master Oct 5, 2018

1 check passed

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

@stuhood stuhood deleted the twitter:dwagnerhall/remoting/status branch Oct 5, 2018

stuhood added a commit to twitter/pants that referenced this pull request Oct 6, 2018

stuhood added a commit that referenced this pull request Oct 6, 2018

Fix merge conflict between two PRs. (#6602)
Fix a merge conflict between #6589 and #6581.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment