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

Increase default pool_size to 4 #1044

Merged
merged 3 commits into from
Apr 28, 2020

Conversation

npwalker
Copy link
Contributor

Solves #1038. pool_size was added as an optional setting
a while ago and people have been using it successfully
in the wild. This commit increases the default to 4
which seems like a generally useful improvement.

Please add all notable changes to the "Unreleased" section of the CHANGELOG.

Solves puppetlabs#1038.  pool_size was added as an optional setting
a while ago and people have been using it successfully
in the wild.  This commit increases the default to 4
which seems like a generally useful improvement.
@npwalker npwalker requested a review from a team April 15, 2020 14:59
@mwaggett
Copy link
Contributor

I'm curious how 4 was decided on.

@npwalker
Copy link
Contributor Author

I chose 4 as two users have indicated they use 5 successfully. The linked issue #1038 shows some time breakdowns for one user's testing.

I'd be willing to compromise to 3 or 2 if we are worried about 4 but given modern computing 4 threads to download and extract modules seems relatively safe. I'd be surprised if it performed worse than 1 in any production scenario.

@adrienthebo
Copy link
Contributor

@npwalker There's a bug in one of the tests; we're copying references to a single object instead of having for objects. Try applying this patch:

diff --git i/spec/unit/action/puppetfile/install_spec.rb w/spec/unit/action/puppetfile/install_spec.rb
index 904983b..f77641d 100644
--- i/spec/unit/action/puppetfile/install_spec.rb
+++ w/spec/unit/action/puppetfile/install_spec.rb
@@ -19,7 +19,9 @@ describe R10K::Action::Puppetfile::Install do
 
   describe "installing modules" do
     let(:modules) do
-      Array.new(4, R10K::Module::Base.new('author/modname', "/some/nonexistent/path/modname", nil))
+      (1..4).map do |idx|
+        R10K::Module::Base.new("author/modname#{idx}", "/some/nonexistent/path/modname#{idx}", nil)
+      end
     end
 
     before do

@adrienthebo
Copy link
Contributor

Regarding the level of concurrency, 4 is a reasonable value. I'm 👍 once the tests are green.

We were copying references to a single object instead of having four
objects
Copy link
Contributor

@reidmv reidmv left a comment

Choose a reason for hiding this comment

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

👍 to upping the default.

@adrienthebo adrienthebo merged commit cf98f9a into puppetlabs:master Apr 28, 2020
justinstoller added a commit to justinstoller/r10k that referenced this pull request May 5, 2020
…ault_pool_size"

This reverts commit cf98f9a, reversing
changes made to 71a7ae7.
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.

5 participants