5439: InProcessTransport optionally adds causal information to status #6968

reggiemcdonald · 2020-04-23T22:58:39Z

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 · 2020-04-23T22:58:41Z

The committers are authorized under a signed CLA.

✅ Reginald McDonald (7f6453a)

ejona86

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).

core/src/main/java/io/grpc/inprocess/InProcessChannelBuilder.java

core/src/main/java/io/grpc/inprocess/InProcessTransport.java

core/src/test/java/io/grpc/inprocess/InProcessTransportTestWithCause.java

…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.

ejona86

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.

core/src/test/java/io/grpc/internal/AbstractTransportTest.java

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.

dapengzhang0 · 2020-05-05T22:27:47Z

core/src/main/java/io/grpc/inprocess/InProcessTransport.java

@@ -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) {


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

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

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

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".

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 · 2020-05-06T18:10:38Z

@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.

inprocess,internal: add ability to pass status cause to client

7f6453a

ejona86 reviewed Apr 24, 2020

View reviewed changes

reggiemcdonald added 3 commits April 26, 2020 00:11

inprocess: remove unused method

e85eac9

Addressing PR feedback, this commit removes the getter from InProcessTransport that is not used in the codebase.

ejona86 reviewed May 4, 2020

View reviewed changes

core/src/test/java/io/grpc/internal/AbstractTransportTest.java Show resolved Hide resolved

ejona86 added the kokoro:run Add this label to a PR to tell Kokoro the code is safe and tests can be run label May 4, 2020

grpc-kokoro removed the kokoro:run Add this label to a PR to tell Kokoro the code is safe and tests can be run label May 4, 2020

ejona86 self-assigned this May 4, 2020

internal: Add back assertnull assertion for server status

4ac7beb

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 approved these changes May 5, 2020

View reviewed changes

ejona86 requested a review from dapengzhang0 May 5, 2020 20:25

ejona86 added the kokoro:run Add this label to a PR to tell Kokoro the code is safe and tests can be run label May 5, 2020

grpc-kokoro removed the kokoro:run Add this label to a PR to tell Kokoro the code is safe and tests can be run label May 5, 2020

dapengzhang0 approved these changes May 5, 2020

View reviewed changes

ejona86 added the kokoro:run Add this label to a PR to tell Kokoro the code is safe and tests can be run label May 6, 2020

grpc-kokoro removed the kokoro:run Add this label to a PR to tell Kokoro the code is safe and tests can be run label May 6, 2020

dapengzhang0 added the kokoro:run Add this label to a PR to tell Kokoro the code is safe and tests can be run label May 6, 2020

ejona86 merged commit 16b6145 into grpc:master May 6, 2020

ejona86 removed the kokoro:run Add this label to a PR to tell Kokoro the code is safe and tests can be run label May 6, 2020

github-actions bot locked as resolved and limited conversation to collaborators Jun 13, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

5439: InProcessTransport optionally adds causal information to status #6968

5439: InProcessTransport optionally adds causal information to status #6968

reggiemcdonald commented Apr 23, 2020 •

edited

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

edited

ejona86 left a comment

ejona86 left a comment

dapengzhang0 May 5, 2020 •

edited

reggiemcdonald May 5, 2020

ejona86 May 5, 2020

dapengzhang0 May 5, 2020

reggiemcdonald May 5, 2020 •

edited

ejona86 commented May 6, 2020

5439: InProcessTransport optionally adds causal information to status #6968

5439: InProcessTransport optionally adds causal information to status #6968

Conversation

reggiemcdonald commented Apr 23, 2020 • edited

linux-foundation-easycla bot commented Apr 23, 2020 • edited

ejona86 left a comment

Choose a reason for hiding this comment

ejona86 left a comment

Choose a reason for hiding this comment

dapengzhang0 May 5, 2020 • edited

Choose a reason for hiding this comment

reggiemcdonald May 5, 2020

Choose a reason for hiding this comment

ejona86 May 5, 2020

Choose a reason for hiding this comment

dapengzhang0 May 5, 2020

Choose a reason for hiding this comment

reggiemcdonald May 5, 2020 • edited

Choose a reason for hiding this comment

ejona86 commented May 6, 2020

reggiemcdonald commented Apr 23, 2020 •

edited

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

edited

dapengzhang0 May 5, 2020 •

edited

reggiemcdonald May 5, 2020 •

edited