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

interop client: provide new flag, --soak_min_time_ms_between_rpcs #5421

Merged
merged 7 commits into from
Jun 24, 2022

Conversation

apolcyn
Copy link
Contributor

@apolcyn apolcyn commented Jun 14, 2022

This flag already exists in the C++ client, and is described under https://github.com/grpc/grpc/blob/master/doc/interop-test-descriptions.md#rpc_soak.

Adding this flag helps to use this client in some tests where we want to run RPCs at a certain QPS.

RELEASE NOTES: none

@apolcyn apolcyn marked this pull request as ready for review June 14, 2022 21:14
@@ -747,6 +751,14 @@ func DoSoakTest(tc testgrpc.TestServiceClient, serverAddr string, dopts []grpc.D
continue
}
fmt.Fprintf(os.Stderr, "soak iteration: %d elapsed_ms: %d succeeded\n", i, latencyMs)
if t != nil {
Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of creating a time.Ticker above, could you consider doing the following here?

if minTimeBetweenRPCs != 0 {
  <-time.After(minTimeBetweenRPCs)
}

With this approach, you wouldn't have to worry about stopping the ticker below.

Also, I'm not quite sure if time.After() returns immediately if it is passed a duration of 0. If that is the case, you would need to check if the duration is non-zero either.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done, nice cleanup!

I made a slight change to your comment, which is to create the time.After channel before performing the RPC, and then wait on it after. This is because the flag is meant to control min time between RPC starts, rather than between finishes and starts.

@apolcyn apolcyn removed their assignment Jun 17, 2022
@easwars easwars added this to the 1.48 Release milestone Jun 17, 2022
@easwars
Copy link
Contributor

easwars commented Jun 22, 2022

Could you please resolve the conflicts and re-assign it to me. I will merge it then. Thanks.

@apolcyn
Copy link
Contributor Author

apolcyn commented Jun 22, 2022

@easwars I've fixed merge conflicts, thanks

@easwars
Copy link
Contributor

easwars commented Jun 22, 2022

@easwars I've fixed merge conflicts, thanks

Looks like you need to run gofmt on your code :(

@apolcyn
Copy link
Contributor Author

apolcyn commented Jun 24, 2022

Looks like you need to run gofmt on your code :(

Done, thanks for catching

@easwars easwars merged commit c075d20 into grpc:master Jun 24, 2022
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Dec 22, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants