-
Notifications
You must be signed in to change notification settings - Fork 4.4k
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
interop: update client for xds testing support #4108
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? |
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But why not just add the goroutine? Why do you need to remove the old goroutine?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If I didn't missing anything, this one is not fixed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you try the full circuit breaking interop tests? You will need to login to a VM and run.
var i int | ||
for range ticker.C { | ||
go func(i int) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Move this to line 401 and don't capture i
in the closure.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Made this change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Manually tested and fixed a bug. Should be good to go.
No description provided.