benchmark/client: add context for cancellation #8614
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #8614 +/- ##
==========================================
+ Coverage 81.21% 81.25% +0.04%
==========================================
Files 416 416
Lines 41002 41002
==========================================
+ Hits 33298 33317 +19
+ Misses 6226 6205 -21
- Partials 1478 1480 +2 🚀 New features to boost your workflow:
|
|
Could you please add "Type: Internal Cleanup" label? Thanks |
There was a problem hiding this comment.
Apologies for the delayed review and thanks for the contribution!
A few main points:
- We should pass the context though function params.
- We should try to derive contexts from other contexts as much as possible instead of using context.Background.
- We can avoid a lot of the
ctx.Err()checks by letting RPCs fail and handling the case when the error had a status of CANCELLED.
|
Thank you very much for the review, it is very helpful! |
| bc.lockingHistograms[idx].histogram = stats.NewHistogram(bc.histogramOptions) | ||
| if poissonLambda == nil { // Closed loop. | ||
| if stream.Context().Err() != nil { | ||
| return | ||
| } | ||
| // Start goroutine on the created mutex and histogram. | ||
| go func(idx int) { | ||
| // TODO: do warm up if necessary. |
There was a problem hiding this comment.
You said
We usually do the check just before performing an asynchronous operation
so I decided to do it like that. If it is not the appropriate way, please tell me
There was a problem hiding this comment.
Since gRPC handles context cancellation and returns a CANCELLED status, this check seems redundant. Considering the context could be cancelled just after we check Context().Err(), checking it here explicitly doesn't buy us much.
|
@wdsjk Please let us know when this is ready for another review pass. |
|
This is ready, review please @easwars @arjan-bal |
| bc.lockingHistograms[idx].histogram = stats.NewHistogram(bc.histogramOptions) | ||
| if poissonLambda == nil { // Closed loop. | ||
| if stream.Context().Err() != nil { | ||
| return | ||
| } | ||
| // Start goroutine on the created mutex and histogram. | ||
| go func(idx int) { | ||
| // TODO: do warm up if necessary. |
There was a problem hiding this comment.
Since gRPC handles context cancellation and returns a CANCELLED status, this check seems redundant. Considering the context could be cancelled just after we check Context().Err(), checking it here explicitly doesn't buy us much.
|
Everything is ready, review please @arjan-bal |
4 participants
Fixes: #8596
Added cancellation by context in benchmark/client instead of custom channel approach. Not sure about returning
ctx.Err()(as mentioned in the original issue under point 4), so please reviewRELEASE NOTES: None