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

interop: implement rpc-behavior for UnaryCall() #6575

Merged
merged 12 commits into from
Sep 27, 2023
Merged

interop: implement rpc-behavior for UnaryCall() #6575

merged 12 commits into from
Sep 27, 2023

Conversation

Aditya-Sood
Copy link
Contributor

@Aditya-Sood Aditya-Sood commented Aug 23, 2023 •
edited by easwars
Loading

@Aditya-Sood
Copy link
Contributor Author

hi @easwars @zasweq
could you please review the changes?

if they look good I'll start on the additional rpc-behavior testcase

@easwars easwars added this to the 1.58 Release milestone Aug 23, 2023
@dfawley dfawley modified the milestones: 1.58 Release, 1.59 Release Aug 24, 2023
zasweq
zasweq requested changes Aug 24, 2023
View reviewed changes
Copy link
Contributor

@zasweq zasweq left a comment

Choose a reason for hiding this comment

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

go.sum doesn't need to be updated like that, and please fix vet. You can run VET_SKIP_PROTO=1 ./vet.sh from the home directory to run the vet script. Also will need to run interop tests against this before merging to make sure this doesn't break anything.

@zasweq zasweq mentioned this pull request Aug 24, 2023
Merged
easwars
easwars reviewed Aug 24, 2023
View reviewed changes
@Aditya-Sood
Copy link
Contributor Author

hi @zasweq and @easwars
thanks for the review, I've implemented the changes in the latest commit

fixed the go.sum issue (I needed to use the -compat flag when running go mod tidy locally)
also all tests run fine with the latest commit

  • I had a query regarding the string constant "rpc-behavior" being declared inline - is the cost of resolving references in go significant enough to prefer avoiding a single definition (which is easier to maintain)?

  • also there are two pending reviews above (concerning the logic for extracting rpc-behaviour metadata) in which I require your clarifications, kindly let me know

thank you for sharing the style guide as well, I will go through it

@easwars easwars assigned zasweq and unassigned Aditya-Sood Aug 29, 2023
easwars
easwars reviewed Aug 31, 2023
View reviewed changes
@easwars
Copy link
Contributor

easwars commented Aug 31, 2023

  • I had a query regarding the string constant "rpc-behavior" being declared inline - is the cost of resolving references in go significant enough to prefer avoiding a single definition (which is easier to maintain)?

It's got less to do with the runtime cost and more to do with cognitive cost when reading the code.

  • also there are two pending reviews above (concerning the logic for extracting rpc-behaviour metadata) in which I require your clarifications, kindly let me know

Are you referring to individual comments or separate PRs? It is not clear to me.

thank you for sharing the style guide as well, I will go through it

Reading through the whole thing in one go can be painful :) and not very useful. I'd recommend slowly incorporating those guidelines into your coding.

zasweq referenced this pull request in grpc/grpc-java Sep 2, 2023
@murgatroid99
xds: Update rpc-behavior handling in interop server
78067cb
zasweq
zasweq requested changes Sep 2, 2023
View reviewed changes
@zasweq zasweq removed their assignment Sep 2, 2023
@zasweq zasweq self-requested a review September 2, 2023 00:30
@arvindbr8
Copy link
Member

@Aditya-Sood let us know if this PR is ready for review again.

@Aditya-Sood
Copy link
Contributor Author

@arvindbr8 yes I've asked for review comments here #6575 (comment)

@arvindbr8 arvindbr8 requested a review from easwars September 5, 2023 17:11
@dfawley dfawley changed the title interop: implements rpc-behavior for UnaryCall() interop: implement rpc-behavior for UnaryCall() Sep 5, 2023
easwars
easwars reviewed Sep 6, 2023
View reviewed changes
@Aditya-Sood Aditya-Sood mentioned this pull request Sep 10, 2023
Merged
@zasweq zasweq removed their assignment Sep 15, 2023
@ginayeh ginayeh assigned zasweq and unassigned easwars Sep 19, 2023
zasweq
zasweq requested changes Sep 19, 2023
View reviewed changes
Copy link
Contributor

@zasweq zasweq left a comment

Choose a reason for hiding this comment

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

One final thing

interop/xds/server/server.go Outdated
mdRPCBehavior := getMetadataValues(ctx, rpcBehaviorMDKey)
var rpcBehaviorMetadata []string
for _, val := range mdRPCBehavior {
splitVals := strings.Split(val, ",")
Copy link
Contributor

Choose a reason for hiding this comment

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

Hello, we discussed this in our gRPC-Go team meeting today. The behavior we agreed upon is actually the trimming of the whitespace, as Java does it. Sorry for the hassle, I know I had you switch it earlier in this PR. Can you please switch it back then I'll go ahead and merge this. After merging, I'll file an issue in the gRPC repo to switch interop tests over to use gRPC-Go as the server. Thanks!

@zasweq zasweq assigned Aditya-Sood and unassigned zasweq Sep 19, 2023
@Aditya-Sood
Copy link
Contributor Author

hi @zasweq, could you please clarify on the above query?

@dfawley dfawley assigned zasweq and unassigned Aditya-Sood Sep 26, 2023
@zasweq
Copy link
Contributor

zasweq commented Sep 27, 2023

Vet proto seems to be failing, this looks good to merge though. Can you please rebase onto master and once it passes I'll merge?

@zasweq zasweq assigned Aditya-Sood and unassigned zasweq Sep 27, 2023
@Aditya-Sood
Increment: Create helper methods to be used in testServiceImpl.UnaryC…
c32eb01 
…all()
@Aditya-Sood
Increment: Modify testServiceImpl.UnaryCall() to return rpc-behavior …
a0bbe3e 
…errors
@Aditya-Sood
Increment: Adds code review changes
65621a6
@Aditya-Sood
Increment: Adds code review changes
6f90b4b
@Aditya-Sood
Increment: Implements rpc-behavior for remaining header values
50d5ff3 
Pending header values: "sleep", "keep-open", "success-on-retry-attempt" and "hostname"
@Aditya-Sood
Increment: Adds code review changes
306cad1 
Uses a switch statement instead of the previous if-else-if ladder for behavior matching
@Aditya-Sood
Bug Fix: Corrects behaviour implementation for 'hostname=<string>' he…
934f8fe 
…ader

And minor code review refactors
@Aditya-Sood
Increment: Adds code review changes
d9bc6e6
@Aditya-Sood
Increment: Adds code review changes
eb011cf
@Aditya-Sood
Minor refactor
a1d0ae7
@Aditya-Sood
Minor Refactor
302e26e
@Aditya-Sood
Increment: Removes whitespace and empty strings for header values
304a7d3
@Aditya-Sood Aditya-Sood force-pushed the 6288-interop-rpc-behaviour branch from a62020c to 304a7d3 Compare September 27, 2023 03:49
@Aditya-Sood
Copy link
Contributor Author

hi @zasweq, vet proto is passing after the rebase

@zasweq zasweq merged commit fd9ef72 into grpc:master Sep 27, 2023
1 check passed
@zasweq zasweq mentioned this pull request Sep 27, 2023
Closed
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Mar 26, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@easwars easwars easwars approved these changes

@zasweq zasweq zasweq approved these changes

Assignees

@Aditya-Sood Aditya-Sood

Labels
Type: Testing
Projects
None yet
Milestone
1.59 Release
Development

Successfully merging this pull request may close these issues.

Interop: implement rpc-behavior server side
5 participants
@Aditya-Sood @easwars @arvindbr8 @zasweq @dfawley