5439: InProcessTransport optionally adds causal information to status #6968
Conversation
|
|
There was a problem hiding this 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).
core/src/main/java/io/grpc/inprocess/InProcessChannelBuilder.java
Outdated
Show resolved
Hide resolved
core/src/main/java/io/grpc/inprocess/InProcessChannelBuilder.java
Outdated
Show resolved
Hide resolved
core/src/test/java/io/grpc/inprocess/InProcessTransportTestWithCause.java
Outdated
Show resolved
Hide resolved
core/src/test/java/io/grpc/inprocess/InProcessTransportTestWithCause.java
Outdated
Show resolved
Hide resolved
…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.
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.
| @@ -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) { | |||
There was a problem hiding this comment.
The method name, the argument, and the javadoc do not match.
There was a problem hiding this comment.
Not quite sure what you mean. Where is the discrepancy?
There was a problem hiding this comment.
I think he's referring to where it says "but stripped of any other information (i.e. cause)"
There was a problem hiding this comment.
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".
There was a problem hiding this comment.
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.
|
@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. |
Closes #5439
Feature request to be able to add causal information to the status in
InProcessTransportAs per @ejona86 suggested:
ServerImplInProcessTransportI also
This is my first open source PR, so any feedback is valued!