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

transport tests: refactor workers in TestMoreStreamsThanOurLimits #2472

Merged
merged 4 commits into from
Aug 17, 2023

Conversation

MarcoPolo
Copy link
Collaborator

The goal of this test is to make a bunch of requests in parallel and hit a peer's limits and eventually make all our requests. The test starts with a certain number of workers and increases them exponentially as long as we haven't seen an error.

The previous code did this as well, but it was extremely subtle. I think this refactor is much clearer in its intent. Instead of having a semaphore that controls the number of workers allowed to run at once, we have dedicated worker goroutines that pull work from a queue. The initial worker (index=0) is special in that it can spawn more workers. This makes it clear who is increasing the worker count and how many parallel workers we have.

This also allows us to set initial worker counts and max worker counts easily, which is useful for debugging some transports.

The core logic of each worker hasn't changed, so it may be useful to review with ?w=1 github flag to ignore whitespace changes.

@marten-seemann marten-seemann changed the title transport tests: TestMoreStreamsThanOurLimits: refactor to increase number of workers clearly transport tests: refactor workers in TestMoreStreamsThanOurLimits Aug 12, 2023
Copy link
Contributor

@marten-seemann marten-seemann left a comment

Choose a reason for hiding this comment

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

Code LGTM, but CI is unhappy.

@marten-seemann marten-seemann merged commit 9bd8502 into master Aug 17, 2023
18 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants