-
Notifications
You must be signed in to change notification settings - Fork 43
Provider get() call optimization #306
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.
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
Conversation
This is one possible way to address #305, but after a lot of consideration, I feel it is the best approach. The complexity should be kept away from the implementer and given Puppet's current design, I believe this approach will do the trick. To anticipate the one big question I expect everyone will have: When Puppet is running as a daemonized agent this still receives a new cache per run. It seems that the type and provider are instantiated as part of the transaction during the Puppet run, following the fork(). When Puppet is running as "puppet resource" or "puppet apply" it is a one-shot mode and so it will be a one-transaction cache regardless. I confirmed this in my testing, but I am unsure as to how to write a test for it. Nevertheless, in all of my testing this is a massive improvement for environments where get() is a resource/time heavy operation. |
ef00677
to
9a56650
Compare
This looks super interesting. @joshcooper Could someone please review this PR? Thanks :) |
Hey @seanmil, maybe you could create an issue in the Puppet issue tracker and link to your PR from there? From my experience this may help to get some resource assigned to a PR. :) |
9a56650
to
66034e4
Compare
Closing and re-opening PR to re-kick testing run. Interested to see if this is green. |
PR has failing/cancelled tests. It also seems to be conflicting with recently merged bugfixes. Needs to be addressed before it can be merged. |
66034e4
to
be96084
Compare
In preparation for adding a caching layer, switch all code accessing the instance variable directly to using the accessors. As part of this, a long standing bug in the RSAPI and an acceptance test was corrected for strict behavior on canonicalization with 'puppet resource'.
This helper class will provide a simple cache for Resource API provider state to minimize the number of get() calls to the provider.
Adding the cache layer allows the provider.get() results to be re-used across multiple Puppet::Resource instantiations for a single actual provider resource within a given Puppet run.
Ensure that remaining data fetched with the provider get() method is cached.
be96084
to
53d764d
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Tested branch against Firewall module to ensure nothing breaks outside our CI.
This PR adds a caching laying which caches the results returned by the provider's get() method to try to actually minimize the calls to get(), per the Puppet Resource API specifications.