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

Jump to bottom

feat: increase default QPS/Burst to 40/80. Allow values to be tunable #1679

Merged
merged 1 commit into from
Dec 3, 2021
Merged

feat: increase default QPS/Burst to 40/80. Allow values to be tunable #1679

merged 1 commit into from
Dec 3, 2021

Conversation

jessesuen
Copy link
Member

Signed-off-by: Jesse Suen jessesuen@gmail.com

@jessesuen jessesuen requested a review from huikang December 2, 2021 00:00
@codecov
Copy link

codecov bot commented Dec 2, 2021 •
edited
Loading

Codecov Report

Merging #1679 (21835c1) into master (9d32c13) will increase coverage by 0.01%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1679      +/-   ##
==========================================
+ Coverage   81.97%   81.99%   +0.01%     
==========================================
  Files         116      116              
  Lines       15929    15929              
==========================================
+ Hits        13058    13061       +3     
+ Misses       2201     2198       -3     
  Partials      670      670
Impacted Files Coverage Δ
utils/defaults/defaults.go 88.02% <ø> (ø)
rollout/temlateref.go 82.16% <0.00%> (+1.62%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9d32c13...21835c1. Read the comment docs.

huikang
huikang approved these changes Dec 2, 2021
View reviewed changes
Copy link
Member

@huikang huikang left a comment

Choose a reason for hiding this comment

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

LGTM

@sonarqubecloud
Copy link

sonarqubecloud bot commented Dec 2, 2021

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
6.0% 6.0% Duplication

agrawroh
agrawroh reviewed Dec 3, 2021
View reviewed changes
cmd/rollouts-controller/main.go
@@ -48,6 +48,8 @@ func newCommand() *cobra.Command {
metricsPort int
healthzPort int
instanceID string
qps float32
Copy link
Member

@agrawroh agrawroh Dec 3, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

@jessesuen Curious if it make sense to have QPS also as an int instead of a float?

Copy link
Member

Choose a reason for hiding this comment

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

I think this is to make it consistent with the client-go type: https://github.com/kubernetes/client-go/blob/d85d0d914a32b2cd96ebfd5f65f2087d6272f70c/rest/config.go#L45

@jessesuen jessesuen merged commit 4d3b992 into argoproj:master Dec 3, 2021
@jessesuen jessesuen deleted the perf-tuning branch December 3, 2021 18:16
jessesuen added a commit that referenced this pull request Dec 3, 2021
@jessesuen
feat: increase default QPS/Burst to 40/80. Allow values to be tunable (…
e41d820 
…#1679)

Signed-off-by: Jesse Suen <jesse@akuity.io>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@agrawroh agrawroh agrawroh left review comments

@huikang huikang huikang approved these changes

@rbreeze rbreeze rbreeze approved these changes

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
@jessesuen @huikang @rbreeze @agrawroh