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.

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

Support retrying gRPCs with chained interceptors #100

Merged
merged 3 commits into from Feb 27, 2018

Conversation

marcwilson-g
Copy link
Contributor

@marcwilson-g marcwilson-g commented Nov 17, 2017

As retries may cause a handler chain to be partially re-executed the
interceptor needs to be reset as the recursion unrolls.

Fix for #110

Possibly regressed as part of
5d4723c

Added tests to the retry_test as this case seemed retry specific, but did feel odd to test the chaining in the retry.

@googlebot
Copy link

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed, please reply here (e.g. I signed it!) and we'll verify. Thanks.


  • If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
  • If your company signed a CLA, they designated a Point of Contact who decides which employees are authorized to participate. You may need to contact the Point of Contact for your company and ask to be added to the group of authorized contributors. If you don't know who your Point of Contact is, direct the project maintainer to go/cla#troubleshoot.
  • In order to pass this check, please resolve this problem and have the pull request author add another comment and the bot will run again.

@codecov-io
Copy link

codecov-io commented Nov 17, 2017

Codecov Report

Merging #100 into master will increase coverage by 0.47%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #100      +/-   ##
==========================================
+ Coverage    73.2%   73.68%   +0.47%     
==========================================
  Files          36       36              
  Lines        1284     1292       +8     
==========================================
+ Hits          940      952      +12     
+ Misses        295      293       -2     
+ Partials       49       47       -2
Impacted Files Coverage Δ
chain.go 77.27% <100%> (+2.27%) ⬆️
retry/retry.go 74.09% <0%> (+1.2%) ⬆️
retry/backoff.go 100% <0%> (+33.33%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d0c54e6...21b739d. Read the comment docs.

@marcwilson-g
Copy link
Contributor Author

I signed it!

@googlebot
Copy link

CLAs look good, thanks!

@changpingc
Copy link

ping @alcore can you review this? thanks

@domgreen
Copy link
Contributor

@marcwilson-g could you update the documentation explicit that the retry will call every interceptor after the retry when a retry happens.
Can you do this in both retry/doc.go and doc.go.

@domgreen
Copy link
Contributor

@yifanzz @mwitkow LGTM apart from updating docs 👍

@marcwilson-g
Copy link
Contributor Author

Back from leave today!
Have added comments in both retry/doc.go and doc.go as suggested (though it wasn't clear if I should also add comments to the .md files too as they seem to contain duplicate content to the doc.go files?

@domgreen
Copy link
Contributor

Hey Marc, sorry for the delay getting back to you. The if you run make it should auto generate the .MD files for you.

As retries may cause a handler chain to be partially re-executed the
interceptor needs to be reset as the recursion unrolls.
@marcwilson-g
Copy link
Contributor Author

@domgreen Done!

Copy link
Contributor

@domgreen domgreen left a comment

Choose a reason for hiding this comment

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

Thanks Marc, will chase someone to accept the PR :)

@domgreen domgreen merged commit f9799d9 into grpc-ecosystem:master Feb 27, 2018
@@ -31,7 +31,9 @@ func ChainUnaryServer(interceptors ...grpc.UnaryServerInterceptor) grpc.UnarySer
return handler(currentCtx, currentReq)
}
curI++
return interceptors[curI](currentCtx, currentReq, info, chainHandler)
resp, err := interceptors[curI](currentCtx, currentReq, info, chainHandler)
curI--

Choose a reason for hiding this comment

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

Why sever side also need curI--?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants