Skip to content
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

5439: InProcessTransport optionally adds causal information to status #6968

Merged
merged 6 commits into from May 6, 2020

Conversation

reggiemcdonald
Copy link
Contributor

@reggiemcdonald reggiemcdonald commented Apr 23, 2020

Closes #5439
Feature request to be able to add causal information to the status in InProcessTransport

As per @ejona86 suggested:

  • Added causal information in ServerImpl
  • Optionally copy that information in InProcessTransport

I also

  • Updated tests
  • Updated factories

This is my first open source PR, so any feedback is valued!

@linux-foundation-easycla
Copy link

@linux-foundation-easycla linux-foundation-easycla bot commented Apr 23, 2020

CLA Check
The committers are authorized under a signed CLA.

Copy link
Member

@ejona86 ejona86 left a comment

It is generally a good idea to include description information in the commit itself, things like why are you making the change and "Addresses #5439". That way in the future you can do things like git blame and understand quickly why code is the way it is (just like how I determined the assertNull(statusCaptor.getValue().getCause()); was actually safe to ignore).

…er.propagateCauseWithStatus

Addresses comment in PR to change method name tranportIncludeStatusCause to something more descriptive: propagateCauseWithStatus. Updates javadoc to better describe what the propagateCauseWithStatus setting does - including the default behaviour of the method.
Addressing PR feedback, this commit removes the getter from InProcessTransport that is not used in the codebase.
…se with status

To address a PR comment, this commit makes the includeCauseWithStatus field of the InProcessTransport final, and adds a single test case to verify that the cause is propagated properly when configured. The field server of AbstractTransportTest had to be made protected so that it could be nullified in the test case for the after each hook.
Copy link
Member

@ejona86 ejona86 left a comment

The AbstractTransportTest.checkClientStatus changes no longer look necessary, but don't seem harmful. Just the one comment about a removed check that doesn't look right.

@ejona86 ejona86 added the kokoro:run label May 4, 2020
@grpc-kokoro grpc-kokoro removed the kokoro:run label May 4, 2020
@ejona86 ejona86 self-assigned this May 4, 2020
This commit addresses PR feedback by adding back a null assertion. Default behaviour for transports is to strip cause from status. So in AbstractTransportTest.clientCancel, server status should have a null cause.
ejona86
ejona86 approved these changes May 5, 2020
@ejona86 ejona86 requested a review from dapengzhang0 May 5, 2020
@ejona86 ejona86 added the kokoro:run label May 5, 2020
@grpc-kokoro grpc-kokoro removed the kokoro:run label May 5, 2020
@@ -849,13 +853,17 @@ public void appendTimeoutInsight(InsightBuilder insight) {
* <p>This is, so that the InProcess transport behaves in the same way as the other transports,
* when exchanging statuses between client and server and vice versa.
*/
private static Status stripCause(Status status) {
private static Status stripCause(Status status, boolean includeCauseWithStatus) {
Copy link
Member

@dapengzhang0 dapengzhang0 May 5, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The method name, the argument, and the javadoc do not match.

Copy link
Contributor Author

@reggiemcdonald reggiemcdonald May 5, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not quite sure what you mean. Where is the discrepancy?

Copy link
Member

@ejona86 ejona86 May 5, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think he's referring to where it says "but stripped of any other information (i.e. cause)"

Copy link
Member

@dapengzhang0 dapengzhang0 May 5, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Where is the discrepancy?

The method name says "strip cause"
The javadoc says "strip something, i.e. cause"
The new argument says "do not strip cause if true".

Copy link
Contributor Author

@reggiemcdonald reggiemcdonald May 5, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok gotcha. I'll update the javadoc and method name

…t.cleanStatus and update javadoc

This commit addresses PR feedback by renaming InProcessTransport.stripCause to InProcessTransport.cleanStatus and updating the javadoc, to better reflect the fact that the status can now optionally include cause.
@ejona86 ejona86 added the kokoro:run label May 6, 2020
@grpc-kokoro grpc-kokoro removed the kokoro:run label May 6, 2020
@dapengzhang0 dapengzhang0 added the kokoro:run label May 6, 2020
@ejona86 ejona86 merged commit 16b6145 into grpc:master May 6, 2020
13 checks passed
@ejona86
Copy link
Member

@ejona86 ejona86 commented May 6, 2020

@reggiemcdonald, thank you!

FYI: for the commits in response to review feedback, (at least in grpc) you don't tend to need to write much of a commit description. We can tend to figure out what is going on, and may not even read the commit description of cleanups.

In the grpc/grpc repo, those cleanup commits are kept in the final history. But in grpc/grpc-java we generally "squash" them all together into one "pristine" commit.

@ejona86 ejona86 removed the kokoro:run label May 6, 2020
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jun 13, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants