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.

Sign up for GitHub

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

Jump to bottom

Call provides? when resolving, reduce number of calls to provides? #3611

Merged
merged 0 commits into from
Jun 30, 2015
Merged

Call provides? when resolving, reduce number of calls to provides? #3611

merged 0 commits into from
Jun 30, 2015

Conversation

jkeiser
Copy link
Contributor

@jkeiser jkeiser commented Jun 29, 2015

provides? is not called on resources or providers unless provides :x is also called - it should, for backcompat purposes.

jaym
jaym reviewed Jun 29, 2015
View reviewed changes
lib/chef/exceptions.rb
@@ -97,6 +97,7 @@ class GroupIDNotFound < ArgumentError; end
class ConflictingMembersInGroup < ArgumentError; end
class InvalidResourceReference < RuntimeError; end
class ResourceNotFound < RuntimeError; end
class ProviderNotFound < ArgumentError; end
Copy link
Contributor

Choose a reason for hiding this comment

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

You're creating a new class because NoProviderAvailable does not inherit from ArgumentError?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was just creating a new class in order to have an analogue to ResourceNotFound. Up until now, that codepath was raising ArgumentError, so I feel like we need it to be a subclass of that--might be overthinking it though.

I'll check on NoProviderAvailable. I'd like the resource and provider one to be named similarly.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Let's just have this throw NoProviderAvailable, screw ArgumentError, and create an alias for either NoResourceAvailable or ProviderNotFound. Got a preference for which?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think it's fine as is

Copy link
Contributor

Choose a reason for hiding this comment

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

But who could be catching some as generic as ArgumentError... I'm torn

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jdm fixed. I think RuntimeError makes marginally more sense. And I like it when names are consistent.

@chef-supermarket chef-supermarket assigned fujin and unassigned sethvargo Jun 29, 2015
jaym
jaym reviewed Jun 29, 2015
View reviewed changes
lib/chef/provider_resolver.rb
provider_priority_map.list_handlers(node, resource.resource_name).flatten(1).uniq
# @api private
def potential_handlers
priority_map.list_handlers(node, resource.resource_name).flatten(1).uniq
Copy link
Contributor

Choose a reason for hiding this comment

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

Did you not memoize intentionally?

@jkeiser jkeiser force-pushed the jk/call_provides_when_resolving branch from ecae5fd to 7970f7b Compare June 29, 2015 18:58
jaym
jaym reviewed Jun 29, 2015
View reviewed changes
lib/chef/resource.rb
@@ -1304,7 +1304,7 @@ def self.sorted_descendants
end
def self.inherited(child)
super
@sorted_descendants = nil
@@sorted_descendants = nil
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't understand why this changed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It was wrong: that code is supposed to reset the cache on the list of descendants when you create a new subclass of Chef::Resource, but it's resetting the wrong variable. The tests failed once I started using sorted_descendants for real.

@lamont-granquist
Copy link
Contributor

massive improvement in my test perf case.

jaym
jaym reviewed Jun 29, 2015
View reviewed changes
lib/chef/resource_resolver.rb
@@ -107,7 +107,7 @@ def list
#
# @api private
def provided_by?(resource_class)
!prioritized_handlers.include?(resource_class)
potential_handlers.include?(resource_class)
Copy link
Contributor

Choose a reason for hiding this comment

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

why was it !prioritized_handers.include? before?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It was wrong--prioritized_handlers is a list of classes filtered by node, while provided_by? is supposed to ignore node and just tell you whether resource_class is involved at all, on any node.

@jaym
Copy link
Contributor

jaym commented Jun 29, 2015

👍

@jkeiser jkeiser force-pushed the jk/call_provides_when_resolving branch from 35c3025 to 422e4f6 Compare June 29, 2015 22:34
@jaym jaym mentioned this pull request Jun 29, 2015
Closed
14 tasks
coderanger
coderanger reviewed Jun 30, 2015
View reviewed changes
lib/chef/resource_resolver.rb
end

protected

def self.priority_map
Chef::Platform::ResourcePriorityMap.instance
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this, and below, be using Chef.resource_priority_map instead of the Singleton API?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can dig it.

@coderanger
Copy link
Contributor

Overall 👍. This also fixes the back-compat detection code to actually work.

mcquin
mcquin reviewed Jun 30, 2015
View reviewed changes
lib/chef/node_map.rb
end
insert_at ||= @map[key].size - 1
@map
insert_at ||= map[key].size - 1
Copy link
Contributor

Choose a reason for hiding this comment

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

insert_at looks unused after this point. Why is it set here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch, I'm guessing it's a remnant.

@chef-supermarket chef-supermarket assigned jaym and unassigned tyler-ball Jun 30, 2015
@chef-supermarket chef-supermarket assigned jonlives and unassigned jaym Jun 30, 2015
@jkeiser jkeiser force-pushed the jk/call_provides_when_resolving branch from db7cb70 to f97bf94 Compare June 30, 2015 17:11
@jkeiser jkeiser force-pushed the jk/call_provides_when_resolving branch from f97bf94 to b2d4c7b Compare June 30, 2015 17:14
@jkeiser jkeiser closed this Jun 30, 2015
@jkeiser jkeiser force-pushed the jk/call_provides_when_resolving branch from b2d4c7b to e1e1e13 Compare June 30, 2015 17:35
@jkeiser jkeiser merged commit e1e1e13 into master Jun 30, 2015
@tyler-ball tyler-ball deleted the jk/call_provides_when_resolving branch June 30, 2015 22:44
@chef chef locked and limited conversation to collaborators Nov 16, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers
No reviews
Assignees

@smurawski smurawski

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet