interop: update client for xds testing support #4108
Conversation
|
This PR only has the proto change now. Add back the client changes? Did you lose them? |
dfawley
changed the title
I had mentioned doing the proto changes separately. Can you still send a PR with only the proto changes? |
It has only the client changes now. The proto changes are merged in #4109. |
| var i int | ||
| for range ticker.C { | ||
| go func(i int) { |
There was a problem hiding this comment.
Why did you move this code out from the goroutine? This won't work if ticker fires faster than the code execution time.
There was a problem hiding this comment.
My understanding is that the server is being updated so that the RPCs will hang long enough for a count on in-flight RPCs (versus RPCs failed due to circuit breaking) to be made. The old goroutine loops through each type of RPC (empty or unary). Now the loop is first and the go call is inside the loop so that each RPC is on its own goroutine and won't stop the RPC of the other type from being called if it hangs. I will update the ticker though so the timing is correct.
There was a problem hiding this comment.
But why not just add the goroutine? Why do you need to remove the old goroutine?
There was a problem hiding this comment.
The goroutine needs to happen on a basis per the elements in cfgs (i.e. per type of RPC to perform). I didn't remove the old goroutine, just moved it inside the for _, cfg := range cfgs {... loop, since previously it was on a per channel basis. I also had to take the logic of building the savedWatchers out since that needed to stay being done on a per channel basis. Otherwise the goroutine after the change should be the same as the one before.
There was a problem hiding this comment.
If I didn't miss anything, we should keep the per-tick PRC. And this is what I think should happen
- for each tick, start
go func(i)to- build
savedWatchers - get the list of configs
- for each config
- start
go func(i, cfg)to run the PRC
- start
- build
There was a problem hiding this comment.
FWIW this LGTM - as far as I can tell, we don't really need the outer goroutine, as the operations now not inside the goroutine are nonblocking and pretty lightweight.
There was a problem hiding this comment.
If I didn't missing anything, this one is not fixed.
There was a problem hiding this comment.
That's true. I was just thinking there's no need for this "diff". And it feels safer the old way.
There was a problem hiding this comment.
That's true. I was just thinking there's no need for this "diff". And it feels safer the old way.
Some kind of change here was necessary. The old code used one goroutine to send RPCs for all configs (serially). These RPCs were not expected to block unless there was a problem. Now some RPCs are expected to block indefinitely, so it's important for each RPC to happen in its own goroutine. This diff moves the go to directly around the RPC itself, where it could block, instead of around the whole ticker invocation. This seems fine to me. The diffs that aren't real diffs because of indent changing are unfortunate but not problematic.
Let's get the manual tests done ASAP so we can submit this!
| var i int | ||
| for range ticker.C { | ||
| go func(i int) { |
There was a problem hiding this comment.
If I didn't miss anything, we should keep the per-tick PRC. And this is what I think should happen
- for each tick, start
go func(i)to- build
savedWatchers - get the list of configs
- for each config
- start
go func(i, cfg)to run the PRC
- start
- build
interop/xds/client/client.go
Outdated
| c := clients[i] | ||
| for _, cfg := range cfgs { | ||
| go func(i int, cfg *rpcConfig) { | ||
| c := clients[i] |
There was a problem hiding this comment.
Move this to line 401 and don't capture i in the closure.
There was a problem hiding this comment.
Made this change.
There was a problem hiding this comment.
This PR LGTM. @GarrettGutierrez1 Have tried the manual runs? How do they look?
| var i int | ||
| for range ticker.C { | ||
| go func(i int) { |
There was a problem hiding this comment.
That's true. I was just thinking there's no need for this "diff". And it feels safer the old way.
No description provided.