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

stress: client logs total num of calls made #6762

Merged
merged 2 commits into from
Nov 6, 2023

Conversation

temawi
Copy link
Contributor

@temawi temawi commented Nov 3, 2023

Needed for an internal, DirectPath related, stress test.

RELEASE NOTES: none

Copy link

codecov bot commented Nov 3, 2023

Codecov Report

Merging #6762 (44a6117) into master (70f1a40) will decrease coverage by 0.05%.
The diff coverage is n/a.

Additional details and impacted files

@temawi
Copy link
Contributor Author

temawi commented Nov 3, 2023

@dfawley, who could review this?

@@ -241,6 +242,7 @@ func performRPCs(gauge *gauge, conn *grpc.ClientConn, selector *weightedRandomTe
interop.DoCustomMetadata(client, grpc.WaitForReady(true))
Copy link
Member

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.

Copy link
Contributor Author

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.

Copy link
Member

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.

Copy link
Contributor Author

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.

stress/client/main.go Outdated Show resolved Hide resolved
stress/client/main.go Outdated Show resolved Hide resolved
@@ -335,6 +337,6 @@ func main() {
close(stop)
Copy link
Member

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.

Copy link
Member

Choose a reason for hiding this comment

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

(#6763 FYI)

Copy link
Contributor Author

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.

@dfawley dfawley added this to the 1.60 Release milestone Nov 6, 2023
@dfawley dfawley merged commit b8d1c76 into grpc:master Nov 6, 2023
13 checks passed
arvindbr8 pushed a commit to arvindbr8/grpc-go that referenced this pull request Nov 7, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 5, 2024
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