On Sun, Jun 5, 2011 at 01:12, R.I.Pienaar <r...@devco.net> wrote:
Hey, cool code. I have some questions that really deserve answers in
the code, though, since I don't want to count on remembering the "why"
in six months time:
> This allows rubygems to be used to extend or override puppet behavior.
>
> Unfortunately the features system uses the autoloader so checking for
> the rubygems feature causes a call loop. It's possible that a more
> elegant way to avoid this exist rather than just checking the @path
> but this did what I needed to do.
>
> Without caching the directories the tests would take forever to run
> and eventually crash my server. Caching seems to avoid the biggest
> of the performance hit.
>
> Signed-off-by: R.I.Pienaar <r...@devco.net>
> ---
> Local-branch: feature/master/7788
> lib/puppet/configurer.rb | 5 +++--
> lib/puppet/parser/compiler.rb | 1 +
> lib/puppet/util/autoload.rb | 19 +++++++++++++++++--
> spec/unit/util/autoload_spec.rb | 10 +++++++++-
> 4 files changed, 30 insertions(+), 5 deletions(-)
>
> diff --git a/lib/puppet/configurer.rb b/lib/puppet/configurer.rb
> index cfeb73a..00c3b18 100644
> --- a/lib/puppet/configurer.rb
> +++ b/lib/puppet/configurer.rb
> @@ -156,9 +156,10 @@ class Puppet::Configurer
> return
> end
> ensure
> - # Make sure we forget the retained module_directories of any autoload
> - # we might have used.
> + # Make sure we forget the retained module_directories and gem search
> + # path of any autoload we might have used.
> Thread.current[:env_module_directories] = nil
> + Thread.current[:gemsearch_directories] = nil
This doesn't make it at all clear *why* this is being done. What goes
wrong if that doesn't happen? Where (as in, specifically, which bits
of code) is it important that this is flushed? Is it just important
to reduce memory consumption, or does it have semantic effects too?
> # Now close all of our existing http connections, since there's no
> # reason to leave them lying open.
> diff --git a/lib/puppet/parser/compiler.rb b/lib/puppet/parser/compiler.rb
> index c1daade..68498ad 100644
> --- a/lib/puppet/parser/compiler.rb
> +++ b/lib/puppet/parser/compiler.rb
> @@ -22,6 +22,7 @@ class Puppet::Parser::Compiler
> # stick until the next entry to this function.
> Thread.current[:known_resource_types] = nil
> Thread.current[:env_module_directories] = nil
> + Thread.current[:gemsearch_directories] = nil
Why do these have the same semantics as the other values there?
> # ...and we actually do the compile now we have caching ready.
> new(node).compile.to_resource
> diff --git a/lib/puppet/util/autoload.rb b/lib/puppet/util/autoload.rb
> index 6537a4a..e61f4cd 100644
> --- a/lib/puppet/util/autoload.rb
> +++ b/lib/puppet/util/autoload.rb
> @@ -124,9 +124,24 @@ class Puppet::Util::Autoload
> searchpath.map { |dir| Dir.glob("#{dir}/*.rb") }.flatten
> end
>
> - # The list of directories to search through for loadable plugins.
> + # features uses the autoloader which causes a loop so avoid trying to
> + # ask Gem for its path during feature loading.
> + #
> + # We're using a per-thread cache based on the code in #module_directories.
Why are we using a per-thread cache?
Otherwise, it looks reasonably good, code-wise.
On the testing front, I don't see any testing in there that tries to
exercise the thread semantics, or verifies the life-cycle assumptions,
or that verifies correct cache management. Given the issues we had
from distant changes breaking those recently, in the area of thread
caching, it would be important to see those.
(If you can't work static tests to verify correctness, it would be
ideal to see internal assertions in the code that verify the cache is
managed correctly – so that when we mess it up in the field we get
good diagnostics, not just randomly delayed catalog compilation or
whatever.)
Daniel
--
⎋ Puppet Labs Developer – http://puppetlabs.com
✉ Daniel Pittman <dan...@puppetlabs.com>
✆ Contact me via gtalk, email, or phone: +1 (877) 575-9775
♲ Made with 100 percent post-consumer electrons
--
You received this message because you are subscribed to the Google Groups
"Puppet Developers" group.
To post to this group, send email to puppet-dev@googlegroups.com.
To unsubscribe from this group, send email to
puppet-dev+unsubscr...@googlegroups.com.
For more options, visit this group at
http://groups.google.com/group/puppet-dev?hl=en.
