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

Limit the number of inflight requests #84

Merged
merged 5 commits into from
Sep 1, 2020
Merged

Limit the number of inflight requests #84

merged 5 commits into from
Sep 1, 2020

Conversation

corneliusweig
Copy link
Owner

Fix #83

@corneliusweig
Copy link
Owner Author

@jebeaudet can you help out to verify if this fixes the problem? For that you need to build this branch and check out if you still experience this issue #83 . The easiest way to build a binary for your arch is make dev in the cloned repository.

I limited the number of inflight requests to 64. If this is still too much, please try to find a concurrency that still works for you via the --max-inflight parameter. You could also try to make this bigger, because I'd like to have this as large as possible. Ketall already is a bit sluggish and limiting the inflight requests makes it even worse.

@jebeaudet
Copy link

jebeaudet commented Aug 30, 2020

Sure thing! I'll test it out tomorrow and let you know. Thanks!

@jebeaudet
Copy link

Works like a charm, I don't get the warnings anymore and everything works as it should.

I've tested it on my fairly large EKS setup with some max-inflight values and here are the results (in terms of concurrency, higher is not always better!) :

--max-inflight=12
real	0m47.966s
user	1m39.120s
sys	0m13.766s

--max-inflight=32
real	0m41.704s
user	2m1.693s
sys	0m17.293s

--max-inflight=64
real	0m39.143s
user	2m1.190s
sys	0m16.395s

--max-inflight=128
real	0m43.899s
user	2m5.514s
sys	0m18.934s

--max-inflight=256
real	0m41.598s
user	1m55.171s
sys	0m16.327s

--max-inflight=512
real	0m43.184s
user	2m10.304s
sys	0m21.093s

64 looks like a sweet spot for me though the differences are probably negligeable in the end.

Thanks again, let me know if I can be of further help 🎉

@corneliusweig
Copy link
Owner Author

Thank you for providing these numbers. I think 64 parallel requests is quite ok, so let's make that the default.

@codecov-commenter
Copy link

Codecov Report

Merging #84 into master will increase coverage by 10.38%.
The diff coverage is 100.00%.

Impacted file tree graph

@@             Coverage Diff             @@
##           master      #84       +/-   ##
===========================================
+ Coverage   42.01%   52.40%   +10.38%     
===========================================
  Files           9        8        -1     
  Lines         388      250      -138     
===========================================
- Hits          163      131       -32     
+ Misses        203       97      -106     
  Partials       22       22               
Impacted Files Coverage Δ
cmd/root.go 69.56% <100.00%> (+0.67%) ⬆️

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 bf57e25...5b01e54. Read the comment docs.

@corneliusweig corneliusweig merged commit 5a11da2 into master Sep 1, 2020
@corneliusweig corneliusweig deleted the w/aws branch September 1, 2020 22:20
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.

ketall with EKS and aws-iam-authenticator leads to socket exhaustion
3 participants