I'm a little confused by this patch, or at least by the last test - does it make it so you can't query both tags and params at the same time, or does it just make it so that tag queries don't also query the tag param?
On Oct 20, 2009, at 11:43 AM, Brice Figureau wrote: > > Mark or anyone running storeconfigs on 0.25: > if you can, it would be great to test this patch to see if there is no > regression anymore and it still works as intended. > > I only tested it on a small system which didn't exhibit the issue > beforehand. > I also manually tested the AR generated SQL queries, and they look > correct. > > If the testers say it's ok, I think it could be great to have this > fix in 0.25.1 > if it's not too late. > > Please review, test and comment :-) > 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 system tags. > 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 fix this issue by either including parameters if the > collection > expression acts on parameters or by including tags if the collection > expression deals with tag. > > Signed-off-by: Brice Figureau <[email protected]> > --- > lib/puppet/parser/collector.rb | 12 ++++-------- > spec/unit/parser/collector.rb | 13 ++++++++++++- > 2 files changed, 16 insertions(+), 9 deletions(-) > > diff --git a/lib/puppet/parser/collector.rb b/lib/puppet/parser/ > collector.rb > index a7f81b4..a2abb6e 100644 > --- a/lib/puppet/parser/collector.rb > +++ b/lib/puppet/parser/collector.rb > @@ -102,19 +102,15 @@ 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 > - end > + # filter only by things we currently search on > + query = {} > + query.merge!( {:include => {:puppet_tags > => :resource_tags}} ) if search =~ /puppet_tags/ > + query.merge!( {:include => {:param_values > => :param_name}} ) if search =~ /param_name/ > > # We're going to collect objects from rails, but we don't > want any > # objects from this host. > diff --git a/spec/unit/parser/collector.rb b/spec/unit/parser/ > collector.rb > index 926033c..53e46c3 100755 > --- a/spec/unit/parser/collector.rb > +++ b/spec/unit/parser/collector.rb > @@ -506,6 +506,7 @@ describe Puppet::Parser::Collector, "when > building its ActiveRecord query for co > end > > it "should return 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} > @@ -518,12 +519,22 @@ describe Puppet::Parser::Collector, "when > building its ActiveRecord query for co > @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[:include] == {:puppet_tags => :resource_tags} > }.returns([...@resource]) > > @collector.evaluate.should == [...@resource] > end > > + it "should not query 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[3] > + options[:include] == {: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] > -- > 1.6.4 > > > > -- Consistency requires you to be as ignorant today as you were a year ago. -- Bernard Berenson --------------------------------------------------------------------- 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 -~----------~----~----~----~------~----~------~--~---
