On Oct 23, 2009, at 2:00 PM, Brice Figureau wrote: > > On 23/10/09 20:32, Luke Kanies wrote: >> 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? > > I didn't get why they were there since they were already in the > :conditions block of 'query' (see the hunk above this one or the > current > code), so I removed them. > > AFAIK the queries still look OK and still return only exported > resources.
Ah; I've looked at the code more closely and I see it now. +1 -- Computers are not intelligent. They only think they are. --------------------------------------------------------------------- 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 -~----------~----~----~----~------~----~------~--~---
