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.

Sign up for GitHub

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

Jump to bottom

Fix issue: grpc-previous-rpc-attempts not added to initial response metadata #9686

Merged
merged 7 commits into from
Jan 17, 2023
Merged

Fix issue: grpc-previous-rpc-attempts not added to initial response metadata #9686

merged 7 commits into from
Jan 17, 2023

Conversation

pandaapo
Copy link
Contributor

Fixes #9641

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

@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Nov 10, 2022 •
edited
Loading

CLA Signed

The committers listed above are authorized under a signed CLA.

@sanjaypujare
Copy link
Contributor

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

@pandaapo
Copy link
Contributor Author

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

@sanjaypujare I have signed the CLA.

@sanjaypujare
Copy link
Contributor

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
Copy link
Contributor Author

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 sanjaypujare requested a review from ejona86 November 17, 2022 22:29
@sanjaypujare
Copy link
Contributor

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
sanjaypujare reviewed Nov 17, 2022
View reviewed changes
stub/src/main/java/io/grpc/stub/ServerCalls.java Outdated
@@ -38,6 +38,9 @@ public final class ServerCalls {
@VisibleForTesting
static final String MISSING_REQUEST = "Half-closed without a request";

static final Metadata.Key<String> GRPC_PREVIOUS_RPC_ATTEMPTS =
Copy link
Contributor

Choose a reason for hiding this comment

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

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
sanjaypujare reviewed Nov 17, 2022
View reviewed changes
stub/src/main/java/io/grpc/stub/ServerCalls.java Outdated
@@ -126,8 +129,12 @@ public ServerCall.Listener<ReqT> startCall(ServerCall<ReqT, RespT> call, Metadat
Preconditions.checkArgument(
call.getMethodDescriptor().getType().clientSendsOneMessage(),
"asyncUnaryRequestCall is only for clientSendsOneMessage methods");
String preRpcAttempts = null;
if (headers.containsKey(GRPC_PREVIOUS_RPC_ATTEMPTS)) {
Copy link
Contributor

@sanjaypujare sanjaypujare Nov 17, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

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
sanjaypujare reviewed Nov 17, 2022
View reviewed changes
stub/src/main/java/io/grpc/stub/ServerCalls.java Outdated
@@ -126,8 +129,12 @@ public ServerCall.Listener<ReqT> startCall(ServerCall<ReqT, RespT> call, Metadat
Preconditions.checkArgument(
call.getMethodDescriptor().getType().clientSendsOneMessage(),
"asyncUnaryRequestCall is only for clientSendsOneMessage methods");
String preRpcAttempts = null;
Copy link
Contributor

Choose a reason for hiding this comment

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

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
sanjaypujare suggested changes Nov 17, 2022
View reviewed changes
Copy link
Contributor

@sanjaypujare sanjaypujare left a comment

Choose a reason for hiding this comment

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

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

@ejona86
Copy link
Member

ejona86 commented Nov 17, 2022

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
Copy link
Contributor

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

@ejona86 ejona86 requested a review from sanjaypujare January 4, 2023 21:36
sanjaypujare
sanjaypujare suggested changes Jan 5, 2023
View reviewed changes
Copy link
Contributor

@sanjaypujare sanjaypujare left a comment

Choose a reason for hiding this comment

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

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

sanjaypujare
sanjaypujare approved these changes Jan 6, 2023
View reviewed changes
Copy link
Contributor

@sanjaypujare sanjaypujare left a comment

Choose a reason for hiding this comment

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

LGTM

@sanjaypujare 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 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
@sanjaypujare
Copy link
Contributor

@ejona86 do you need to approve as well?

@sanjaypujare
Copy link
Contributor

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

@ejona86 ejona86 self-requested a review January 17, 2023 22:13
@ejona86 ejona86 merged commit 0f4b767 into grpc:master Jan 17, 2023
@ejona86
Copy link
Member

ejona86 commented Jan 17, 2023

Thank you!

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Apr 18, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@sanjaypujare sanjaypujare sanjaypujare approved these changes

@ejona86 ejona86 ejona86 approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

grpc-previous-rpc-attempts not added to initial response metadata
4 participants
@pandaapo @sanjaypujare @ejona86 @grpc-kokoro