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

Auto-upgrade/downgrade implicit concurrency with CPU changes #5196

Merged
merged 4 commits into from Oct 31, 2022

Conversation

inlined
Copy link
Member

@inlined inlined commented Oct 31, 2022

Potential fix for #1288 if we decide to take it. We can decide to:

  1. Auto downgrade from 80 concurrents to 1 concurrent, but not auto-upgrade (may confuse users, but avoids race condition bugs)
  2. Apply the 1/80 concurrent defaults whenever concurrency is not specified (most in-line with other fields)
  3. Keep the current behavior that concurrency is only set implicitly on first deploy

@codecov-commenter
Copy link

codecov-commenter commented Oct 31, 2022

Codecov Report

Base: 56.22% // Head: 56.22% // Increases project coverage by +0.00% 🎉

Coverage data is based on head (2018d2e) compared to base (87e8f0c).
Patch coverage: 42.85% of modified lines in pull request are covered.

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #5196   +/-   ##
=======================================
  Coverage   56.22%   56.22%           
=======================================
  Files         308      308           
  Lines       20752    20757    +5     
  Branches     4206     4210    +4     
=======================================
+ Hits        11667    11670    +3     
  Misses       8071     8071           
- Partials     1014     1016    +2     
Impacted Files Coverage Δ
src/deploy/functions/prepare.ts 30.39% <42.85%> (+0.74%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@inlined
Copy link
Member Author

inlined commented Oct 31, 2022

Updated to also upgrade concurrency when it is not specified and CPU grows to support it.

Comment on lines 330 to 331
// turn off concurrency. We'll not do the opposite and turn on concurrency
// if there are >1 CPU because this could expose race conditions. They
// either need to start on concurrency where they've always needed to
// handle race conditions, or they should explicitly enable.
// We'll hanndle this in setCpuAndConcurrency
Copy link
Contributor

Choose a reason for hiding this comment

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

This comment seems inaccurate w/ latest implementation. We do upgrade concurrency regardless, ignoring race condition.

@inlined inlined force-pushed the inlined.auto-concurrency-downgrade branch from 057e4b4 to f4caad0 Compare October 31, 2022 21:28
@inlined inlined enabled auto-merge (squash) October 31, 2022 21:30
@inlined inlined changed the title Auto-downgrade implicit concurrency Auto-upgrade/downgrade implicit concurrency with CPU changes Oct 31, 2022
@inlined inlined merged commit 60f6d66 into master Oct 31, 2022
taeold added a commit that referenced this pull request Nov 2, 2022
Concurrency will aggressively default to 80 per #5196.
taeold added a commit that referenced this pull request Nov 3, 2022
Concurrency will aggressively default to 80 per #5196.
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.

None yet

4 participants