Fix issue: grpc-previous-rpc-attempts not added to initial response metadata by pandaapo · Pull Request #9686 · grpc/grpc-java

pandaapo · 2022-11-10T23:50:03Z

Fixes #9641

Add grpc-previous-rpc-attempts to headers when response metadata is initialized.

…etadata

linux-foundation-easycla · 2022-11-10T23:50:07Z

The committers listed above are authorized under a signed CLA.

✅ login: pandaapo (356626c)

sanjaypujare · 2022-11-11T00:02:34Z

You will have to sign the CLA before we can consider this PR

pandaapo · 2022-11-13T01:30:56Z

You will have to sign the CLA before we can consider this PR

@sanjaypujare I have signed the CLA.

sanjaypujare · 2022-11-13T20:43:23Z

How does it fix #9641 ? That issue talks about adding grpc-previous-rpc-attempts to the response headers (i.e. to ServerStreams) whereas this makes a small enhancement to the ClientStream when previousAttemptCount is 0.

pandaapo · 2022-11-17T22:07:52Z

How does it fix #9641 ? That issue talks about adding grpc-previous-rpc-attempts to the response headers (i.e. to ServerStreams) whereas this makes a small enhancement to the ClientStream when previousAttemptCount is 0.

I misunderstood the author's intention. I tried to fix it again.

sanjaypujare · 2022-11-17T22:52:18Z

How does it fix #9641 ? That issue talks about adding grpc-previous-rpc-attempts to the response headers (i.e. to ServerStreams) whereas this makes a small enhancement to the ClientStream when previousAttemptCount is 0.

I misunderstood the author's intention. I tried to fix it again.

I have sought clarification in #9641 (comment) . I think @ejona86 was suggesting addinggrpc-previous-rpc-attempts to response headers within RetriableStream and not ServerStream as I had suggested earlier. Not sure what other languages do (if they do) and what the intent was in the gRFC since "Likewise, the client will receive the number of retry attempts made when receiving the results of an RPC" is kind of ambiguous although one would think it implies "the client will receive ... from server"

sanjaypujare · 2022-11-17T22:59:27Z

  @VisibleForTesting
  static final String MISSING_REQUEST = "Half-closed without a request";

+  static final Metadata.Key<String> GRPC_PREVIOUS_RPC_ATTEMPTS =


It will be nice to have a single definition shared with RetriableStream . The current one RetriableStream.GRPC_PREVIOUS_RPC_ATTEMPTS can be moved up to the common base class Stream then we can eliminate this?

sanjaypujare · 2022-11-17T23:06:06Z

          call.getMethodDescriptor().getType().clientSendsOneMessage(),
          "asyncUnaryRequestCall is only for clientSendsOneMessage methods");
+      String preRpcAttempts = null;
+      if (headers.containsKey(GRPC_PREVIOUS_RPC_ATTEMPTS)) {


Why do we need this check? get returns null if not present, so why can't we do :

String preRpcAttempts = headers.get(GRPC_PREVIOUS_RPC_ATTEMPTS);

Same for line 241 (StreamingServerCallHandler.startCall)

sanjaypujare · 2022-11-17T23:12:18Z

      Preconditions.checkArgument(
          call.getMethodDescriptor().getType().clientSendsOneMessage(),
          "asyncUnaryRequestCall is only for clientSendsOneMessage methods");
+      String preRpcAttempts = null;


previousRpcAttempts (or at least prevRpcAttempts) as a name is much better than preRpcAttempts since pre as a short form of previous is not common

sanjaypujare

There is not test case to test the added functionality. That will need to be there. Also added a few comments.

ejona86 · 2022-11-17T23:26:51Z

Not sure what other languages do (if they do) and what the intent was in the gRFC since

The intent was client-only, where the client library injects the value for the client application to see. Yes, it is strange. But that's what it is, since there are few ways for the library to communicate with the application.

sanjaypujare · 2022-11-17T23:34:56Z

In that case @pandaapo you will have to change this PR to add the trailer in RetriableStream on the response side. Also since the information is locally available you don't need to parse the request headers like in case of ServerCalls implementation. Pls make sure to add a test case

sanjaypujare

Suggested changes in main code and test code. After that will approve

sanjaypujare

LGTM

sanjaypujare · 2023-01-06T18:12:58Z

@ejona86 do you need to approve as well?

sanjaypujare · 2023-01-10T21:54:49Z

@ejona86 you also need to approve it before we merge it?

ejona86 · 2023-01-17T23:34:08Z

Thank you!

Fix issue: grpc-previous-rpc-attempts not added to initial response m…

356626c

…etadata

pandaapo added 2 commits November 16, 2022 10:32

Add grpc-previous-rpc-attempts to the response headers

9511d98

Fix checkstyle error.

146a2f9

sanjaypujare requested a review from ejona86 November 17, 2022 22:29

sanjaypujare reviewed Nov 17, 2022

View reviewed changes

sanjaypujare suggested changes Nov 17, 2022

View reviewed changes

pandaapo added 2 commits November 19, 2022 16:48

Add previous retry attempts to reponse header.

be73fc3

Revert change of ServerCalls.

a6d2ce5

sanjaypujare reviewed Nov 22, 2022

View reviewed changes

Comment thread core/src/test/java/io/grpc/internal/RetriableStreamTest.java Outdated

sanjaypujare reviewed Nov 22, 2022

View reviewed changes

Comment thread core/src/test/java/io/grpc/internal/RetriableStreamTest.java

sanjaypujare reviewed Nov 22, 2022

View reviewed changes

Comment thread core/src/main/java/io/grpc/internal/RetriableStream.java Outdated

Modify as review.

e7f58d6

ejona86 reviewed Jan 4, 2023

View reviewed changes

Comment thread core/src/main/java/io/grpc/internal/RetriableStream.java

ejona86 requested a review from sanjaypujare January 4, 2023 21:36

sanjaypujare reviewed Jan 5, 2023

View reviewed changes

Comment thread core/src/test/java/io/grpc/internal/RetriableStreamTest.java Outdated

sanjaypujare reviewed Jan 5, 2023

View reviewed changes

Comment thread core/src/test/java/io/grpc/internal/RetriableStreamTest.java Outdated

sanjaypujare suggested changes Jan 5, 2023

View reviewed changes

Modify as review.

ce138a8

sanjaypujare approved these changes Jan 6, 2023

View reviewed changes

sanjaypujare added the kokoro:run Add this label to a PR to tell Kokoro the code is safe and tests can be run label Jan 6, 2023

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 Jan 6, 2023

ejona86 self-requested a review January 17, 2023 22:13

ejona86 approved these changes Jan 17, 2023

View reviewed changes

ejona86 merged commit 0f4b767 into grpc:master Jan 17, 2023

github-actions Bot locked as resolved and limited conversation to collaborators Apr 18, 2023

Conversation

pandaapo commented Nov 10, 2022

Uh oh!

linux-foundation-easycla Bot commented Nov 10, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sanjaypujare commented Nov 11, 2022

Uh oh!

pandaapo commented Nov 13, 2022

Uh oh!

sanjaypujare commented Nov 13, 2022

Uh oh!

pandaapo commented Nov 17, 2022

Uh oh!

sanjaypujare commented Nov 17, 2022

Uh oh!

sanjaypujare Nov 17, 2022

Choose a reason for hiding this comment

Uh oh!

sanjaypujare Nov 17, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sanjaypujare Nov 17, 2022

Choose a reason for hiding this comment

Uh oh!

sanjaypujare left a comment

Choose a reason for hiding this comment

Uh oh!

ejona86 commented Nov 17, 2022

Uh oh!

sanjaypujare commented Nov 17, 2022

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

sanjaypujare left a comment

Choose a reason for hiding this comment

Uh oh!

sanjaypujare left a comment

Choose a reason for hiding this comment

Uh oh!

sanjaypujare commented Jan 6, 2023

Uh oh!

sanjaypujare commented Jan 10, 2023

Uh oh!

ejona86 commented Jan 17, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

linux-foundation-easycla Bot commented Nov 10, 2022 •

edited

Loading

sanjaypujare Nov 17, 2022 •

edited

Loading