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.

Sign up for GitHub

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

[test] Avoid Rate Limit Exceeded #24931

Merged
merged 8 commits into from
Feb 16, 2021
Merged

[test] Avoid Rate Limit Exceeded #24931

Changes from 3 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
5f76356
[test] Avoid Rate Limit Exceeded
oliviertassinari Feb 14, 2021
9425c9a
prettier
oliviertassinari Feb 14, 2021
18f0f2c
review
oliviertassinari Feb 14, 2021
034bde9
explain magic number
oliviertassinari Feb 14, 2021
f20a465
explain magic number
oliviertassinari Feb 14, 2021
c763987
explain why -1
oliviertassinari Feb 15, 2021
700af5c
improve variable name
oliviertassinari Feb 15, 2021
b1f8b8e
more magic numbers
oliviertassinari Feb 15, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 9 additions & 0 deletions test/karma.conf.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,10 @@ const browserStack = {

process.env.CHROME_BIN = playwright.chromium.executablePath();

// per second, https://www.browserstack.com/docs/automate/api-reference/selenium/introduction#rest-api-projects
const MAX_REQUEST_BROWSERSTACK = 1600 / (60 * 5);
oliviertassinari marked this conversation as resolved.
Show resolved Hide resolved
const MAX_KARMA_CONCURRENT_BUILD = 80 / 6;
Copy link
Member

Choose a reason for hiding this comment

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

The maximum amount of concurrent test_browser jobs is 80.

I'm not following how you leap from "CircleCI accepts up to 80 concurrent builds" to "for each PR, 6 concurrent builds are used". How does a PR factor in here?

If you're estimating at some point then this should be its own magic number so that we can easily adjust later. Not baked into a well defined constant.

Seems to me we don't actually care about the CircleCI concurrency (or rather just as an upper bound for verification). Instead we're just trying to handle the maximum number of parallel test_browser jobs. I'd suggest just going with "10" for now and see how it goes.

Copy link
Member Author

Choose a reason for hiding this comment

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

6 comes from the number if items in:

Capture d’écran 2021-02-15 à 11 49 09

Ok, let's use another magic number :)

Copy link
Member

Choose a reason for hiding this comment

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

6 comes from the number if items in:

Why is the number of jobs relevant? In a review I'm not interested in the what. I can read code for myself. I need to know the reason for why did it that way because that is not obvious.

Copy link
Member Author

@oliviertassinari oliviertassinari Feb 16, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Let's consider the following case:

  • 20 open PRs at the same time, a condition that can hit the maximum build capacity.
  • CircleCI can process all the jobs for the first step checkout in parallel (because 20 < 83)
  • CircleCI completes all the jobs for the checkout step, more or less at the same time
  • CircleCI moves to the next steps in the pipelines. This time he has to handle 6 x 20 = 120 jobs, 20 of these jobs are test_browser-1. He picks 83 jobs randomly inside these 120 jobs. test_browser-1, test_unit-1, test_static, etc. all the jobs have an independent, equal probability to be picked. So out of the 83 jobs, 83/6 are the step test_browser-1.
  • We are now running ~14 test_browser-1 jobs at the same time.

=> 83/6 is the maximum parallelism Karma can hit (assuming the above simplification of the problem).


// Karma configuration
module.exports = function setKarmaConfig(config) {
const baseConfig = {
Expand Down Expand Up @@ -144,6 +148,11 @@ module.exports = function setKarmaConfig(config) {
},
},
};

// default 1000, Avoid Rate Limit Exceeded
newConfig.pollingTimeout =
((MAX_KARMA_CONCURRENT_BUILD * (newConfig.browsers.length - 1)) / MAX_REQUEST_BROWSERSTACK) *
oliviertassinari marked this conversation as resolved.
Show resolved Hide resolved
1000;
}

config.set(newConfig);
Expand Down