Skip to content

feat: add concurrency flag to create and update#316

Merged
MichaelGoberling merged 2 commits intomasterfrom
add-concurrency-option
Jul 20, 2023
Merged

feat: add concurrency flag to create and update#316
MichaelGoberling merged 2 commits intomasterfrom
add-concurrency-option

Conversation

@MichaelGoberling
Copy link
Copy Markdown
Contributor

@MichaelGoberling MichaelGoberling commented Jul 19, 2023 •
edited
Loading

Description

Add a new flag for setting concurrency of actions

Default, min, and max taken from Runtime System Settings

Motivation and Context

Trying to set concurrency of a blackbox action, which can't be managed with app.config.yaml

How Has This Been Tested?

Locally linked plugin, npm run test

Screenshots (if appropriate):

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • I have signed the Adobe Open Source CLA.
  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have read the CONTRIBUTING document.
  • I have added tests to cover my changes.
  • All new and existing tests passed.

@codecov
Copy link
Copy Markdown

codecov Bot commented Jul 19, 2023 •
edited
Loading

Codecov Report

Merging #316 (78eb43d) into master (3968c92) will not change coverage.
The diff coverage is 100.00%.

@@            Coverage Diff            @@
##            master      #316   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           66        66           
  Lines         1749      1752    +3     
  Branches       367       369    +2     
=========================================
+ Hits          1749      1752    +3
Files Changed Coverage Δ
src/commands/runtime/action/create.js 100.00% <100.00%> (ø)

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

pru55e11
pru55e11 approved these changes Jul 19, 2023
View reviewed changes
Copy link
Copy Markdown

@pru55e11 pru55e11 left a comment

Choose a reason for hiding this comment

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

overall looks good to me. How'd you come up with the max 500 concurrency? Should we load test this and adjust as needed?

@purplecabbage
Copy link
Copy Markdown
Member

purplecabbage commented Jul 19, 2023

lgtm! Do we have a similar test in the app plugin for deploying from app.config.yaml?
My preference would be that these limits are enforced by the server when we attempt to deploy/update the action, so we do not need to store these values in multiple places; BUT I do not foresee these changing anytime soon, so I think its fine.

@MichaelGoberling
Copy link
Copy Markdown
Contributor Author

MichaelGoberling commented Jul 19, 2023

@purplecabbage

I don't think we do this sort of validation in the app plugin, so there's no similar tests

Agreed on the server side enforcement... Let me spend some time seeing if each of the min/max errors coming back from openwhisk are sufficient and we can remove the client side checking

@MichaelGoberling
Copy link
Copy Markdown
Contributor Author

MichaelGoberling commented Jul 19, 2023

Yea I think the server responses seem fine, let me remove the client side checking here and then make another pr for the other flags in this command

Max exceeded

➜  runtime-memory-test git:(main) ✗ aio runtime action create test-1 --concurrency=5000 ./actions/generic/index.js 
 ›   Error: failed to create the action: the request content was malformed:
 ›   requirement failed: concurrency 5000 exceeds allowed threshold of 500 (400 Bad Request)
 ›    specify --verbose flag for more information

Min exceeded

➜  runtime-memory-test git:(main) ✗ aio runtime action create test-1 --concurrency=-1 ./actions/generic/index.js
 ›   Error: failed to create the action: the request content was malformed:
 ›   requirement failed: concurrency -1 below allowed threshold of 1 (400 Bad Request)
 ›    specify --verbose flag for more information

@MichaelGoberling
Copy link
Copy Markdown
Contributor Author

MichaelGoberling commented Jul 19, 2023

Client side validation removed. Also opened a new PR doing this for other commands: #317

@MichaelGoberling MichaelGoberling merged commit ea0da26 into master Jul 20, 2023
@MichaelGoberling MichaelGoberling deleted the add-concurrency-option branch July 20, 2023 14:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@shazron shazron shazron approved these changes

@purplecabbage purplecabbage purplecabbage approved these changes

@pru55e11 pru55e11 pru55e11 approved these changes

@moritzraho moritzraho Awaiting requested review from moritzraho

@amulyakashyap09 amulyakashyap09 Awaiting requested review from amulyakashyap09

@arjuncooliitr arjuncooliitr Awaiting requested review from arjuncooliitr

@sandeep-paliwal sandeep-paliwal Awaiting requested review from sandeep-paliwal

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@MichaelGoberling @purplecabbage @shazron @pru55e11