-
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
stress: client logs total num of calls made #6762
Conversation
@dfawley, who could review this? |
@@ -241,6 +242,7 @@ func performRPCs(gauge *gauge, conn *grpc.ClientConn, selector *weightedRandomTe | |||
interop.DoCustomMetadata(client, grpc.WaitForReady(true)) |
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.
I don't think we want WaitForReady
anymore. There are probably hysterical raisins for why it was once there, but it can be removed now. Or in a later PR; either 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.
Later PR... Same reasons as with moving to under /interop
.
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 move all of this stuff under /interop
(i.e. cd grpc-go; mv stress interop/
)? I don't think it really wants to be a top-level thing.
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.
I can do that, but rather in a later PR so that this commit stays focused on one thing.
@@ -335,6 +337,6 @@ func main() { | |||
close(stop) |
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.
Optional for this PR but would be nice to fix up soon if we are reviving this:
More natural and simpler would be:
ctx := context.Background
if *testDurationSecs > 0 {
ctx, cancel = context.WithTimeout(ctx, *testDurationSecs * time.Second)
defer cancel()
}
........
performRPCs(ctx, g, conn, testSelector)
......
func performRPCs() {
....
for ctx.Err() == nil {
....
}
}
Even better would be to then pass ctx
to interop.DoBlah
but that probably affects other things too. But it's following a bad practice and should be cleaned up one day. I'll file an issue for that.
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.
(#6763 FYI)
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.
I need to chat with you a bit more about this, not changing in this PR.
Needed for an internal, DirectPath related, stress test.
RELEASE NOTES: none