On Oct 22, 2009, at 11:13 AM, Brice Figureau wrote:
>
> Hi,
>
> Please disregard the previous version of this patch which was plain
> wrong.
> Mainly due to the fact that I thought ":include" was a synonim of
> ":joins"
> in Active Record parlance.
> This one is now correct and should generate the correct query.
>
> Marc: can you try this one and report success/failure?
>
> Please review and comment,
>
> Thanks,
> Brice
>
> Original Commit Msg:
>
> f9516d introduced a change in the way the user tags are persisted
> to the database: user tags are now treated as regular tags (they are
> stored to the tags table).
> Thus this commit changed the AR collector request to also look at the
> tags tables when collecting.
>
> Unfortunately this added a performance regression since tag request
> were still importing the resources parameters tables and AR was
> issuing a large request which was returning all the resource
> parameters
> joined with the tags.
>
> This commit fixes the AR request to join to the needed table, instead
> of doing an include. Including (ie eager loading) parameter values was
> not working for resource parameters anyway since at least 0.24 because
> searching by parameter add a constraint to the joins and only the
> searched parameter was returned instead of all parameter for a given
> exported resource. So on a performance standpoint this new code should
> be as fast 0.24 was.
>
> Signed-off-by: Brice Figureau <[email protected]>
> ---
> lib/puppet/parser/collector.rb | 22 ++++++++++++++--------
> spec/unit/parser/collector.rb | 29 ++++++++++++++++++++---------
> 2 files changed, 34 insertions(+), 17 deletions(-)
>
> diff --git a/lib/puppet/parser/collector.rb b/lib/puppet/parser/
> collector.rb
> index a7f81b4..a6763c4 100644
> --- a/lib/puppet/parser/collector.rb
> +++ b/lib/puppet/parser/collector.rb
> @@ -102,18 +102,24 @@ class Puppet::Parser::Collector
> raise Puppet::DevError, "Cannot collect resources for a nil
> host" unless @scope.host
> host = Puppet::Rails::Host.find_by_name(@scope.host)
>
> - query = {:include => {:param_values => :param_name}}
> -
> search = "(exported=? AND restype=?)"
> values = [true, @type]
>
> search += " AND (%s)" % @equery if @equery
>
> - # this is a small performance optimisation
> - # the tag mechanism involves 3 more joins, which are
> - # needed only if we query on tags.
> - if search =~ /puppet_tags/
> - query[:include][:puppet_tags] = :resource_tags
> + # note:
> + # we're not eagerly including any relations here because
> + # it can creates so much objects we'll throw out later.
> + # We used to eagerly include param_names/values but the way
> + # the search filter is built ruined those efforts and we
> + # were eagerly loading only the searched parameter and not
> + # the other ones.
> + query = {}
> + case search
> + when /puppet_tags/
> + query = {:joins => {:resource_tags => :puppet_tag}}
> + when /param_name/
> + query = {:joins => {:param_values => :param_name}}
> end
>
> # We're going to collect objects from rails, but we don't
> want any
> @@ -139,7 +145,7 @@ class Puppet::Parser::Collector
> # and such, we'll need to vary the conditions, but this
> works with no
> # attributes, anyway.
> time = Puppet::Util.thinmark do
> - Puppet::Rails::Resource.find(:all, @type, true,
> query).each do |obj|
> + Puppet::Rails::Resource.find(:all, query).each do |obj|
What happened to the use of @type and 'exported' to reduce the number
of resources returned? Won't this return all resources, regardless of
whether they're exported or the right type?
> if resource = exported_resource(obj)
> count += 1
> resources << resource
> diff --git a/spec/unit/parser/collector.rb b/spec/unit/parser/
> collector.rb
> index 926033c..7f88bf7 100755
> --- a/spec/unit/parser/collector.rb
> +++ b/spec/unit/parser/collector.rb
> @@ -498,35 +498,46 @@ describe Puppet::Parser::Collector, "when
> building its ActiveRecord query for co
>
> Puppet
> ::Rails::Host.expects(:find_by_name).with(@scope.host).returns(@host)
>
> Puppet::Rails::Resource.stubs(:find).with { |*arguments|
> - options = arguments[3]
> + options = arguments[1]
> options[:conditions][0] =~ /^host_id != \?/ and
> options[:conditions][1] == 5
> }.returns([...@resource])
>
> @collector.evaluate.should == [...@resource]
> end
>
> - it "should return parameter names, parameter values when
> querying ActiveRecord" do
> + it "should join with parameter names, parameter values when
> querying ActiveRecord" do
> + @collector.equery = "param_names.name = title"
> Puppet::Rails::Resource.stubs(:find).with { |*arguments|
> - options = arguments[3]
> - options[:include] == {:param_values => :param_name}
> + options = arguments[1]
> + options[:joins] == {:param_values => :param_name}
> }.returns([...@resource])
>
> @collector.evaluate.should == [...@resource]
> end
>
> - it "should return tags when querying ActiveRecord with a tag
> exported query" do
> + it "should join with tag tables when querying ActiveRecord with
> a tag exported query" do
> @collector.equery = "puppet_tags.name = test"
> Puppet::Rails::Resource.stubs(:find).with { |*arguments|
> - options = arguments[3]
> - options[:include] == {:param_values
> => :param_name, :puppet_tags => :resource_tags}
> + options = arguments[1]
> + options[:joins] == {:resource_tags => :puppet_tag}
> }.returns([...@resource])
>
> @collector.evaluate.should == [...@resource]
> end
>
> + it "should not join parameters when querying ActiveRecord with
> a tag exported query" do
> + @collector.equery = "puppet_tags.name = test"
> + Puppet::Rails::Resource.stubs(:find).with { |*arguments|
> + options = arguments[1]
> + options[:joins] == {:param_values => :param_name}
> + }.returns([...@resource])
> +
> + @collector.evaluate.should be_false
> + end
> +
> it "should only search for exported resources with the matching
> type" do
> Puppet::Rails::Resource.stubs(:find).with { |*arguments|
> - options = arguments[3]
> + options = arguments[1]
> options[:conditions][0].include?("(exported=? AND
> restype=?)") and options[:conditions][1] == true and
> options[:conditions][2] == "Mytype"
> }.returns([...@resource])
>
> @@ -536,7 +547,7 @@ describe Puppet::Parser::Collector, "when
> building its ActiveRecord query for co
> it "should include the export query if one is provided" do
> @collector.equery = "test = true"
> Puppet::Rails::Resource.stubs(:find).with { |*arguments|
> - options = arguments[3]
> + options = arguments[1]
> options[:conditions][0].include?("test = true")
> }.returns([...@resource])
>
> --
> 1.6.4
>
>
> >
--
It is better to sleep on things beforehand than lie awake about them
afterward. -- Baltasar Gracian
---------------------------------------------------------------------
Luke Kanies | http://reductivelabs.com | http://madstop.com
--~--~---------~--~----~------------~-------~--~----~
You received this message because you are subscribed to the Google Groups
"Puppet Developers" group.
To post to this group, send email to [email protected]
To unsubscribe from this group, send email to
[email protected]
For more options, visit this group at
http://groups.google.com/group/puppet-dev?hl=en
-~----------~----~----~----~------~----~------~--~---
