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

[test] Avoid Rate Limit Exceeded #24931

Merged
merged 8 commits into from
Feb 16, 2021

Conversation

test/karma.conf.js Outdated Show resolved Hide resolved
@mui-pr-bot
Copy link

mui-pr-bot commented Feb 14, 2021

No bundle size changes

Generated by 🚫 dangerJS against b1f8b8e

Copy link
Member

@eps1lon eps1lon left a comment

Choose a reason for hiding this comment

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

Please reduce the amount of magic numbers as discussed.

const MAX_REQUEST_BROWSERSTACK = 1600 / (60 * 5);
// Estimate the max number of concurrent karma builds.
// CircleCI accepts up to 80 concurrent builds, for each PR, 6 concurrent builds are used.
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

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).

test/karma.conf.js Outdated Show resolved Hide resolved
test/karma.conf.js Outdated Show resolved Hide resolved
@oliviertassinari oliviertassinari merged commit 51316ee into mui:next Feb 16, 2021
@oliviertassinari oliviertassinari deleted the browser-stack-rate-limit branch February 16, 2021 19:01
const MAX_REQUEST_PER_SECOND_BROWSERSTACK = 1600 / (60 * 5);
// Estimate the max number of concurrent karma builds
// For each PR, 6 concurrent builds are used, only one is usng BrowserStack.
const AVERAGE_KARMA_BUILD = 1 / 6;
Copy link
Member

Choose a reason for hiding this comment

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

What does "AVERAGE_KARMA_BUILD" mean?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants