-
Notifications
You must be signed in to change notification settings - Fork 42
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
Use cached get() results to limit calls to get() #346
Open
amandaharth
wants to merge
2
commits into
puppetlabs:main
Choose a base branch
from
amandaharth:use-cache-to-limit-get
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
59772ca
to
fc129b7
Compare
Could you rebase your PR instead of pulling (and creating merge commits in your PR) |
ffb883e
to
b4336f6
Compare
@puppetlabs/phoenix any more updates on this PR from you guys? |
@joshcooper Hey, this PR looks good on our end but wanted to make sure it was good on yours as well? |
My understanding... there may be cases I haven't considered. In my testing, with simple_get_filter not implemented, removing the names.nil? checks stopped get(context) being called again for every resource which had pending changes. All resources of the given type were collected from the system with one get() call and cached. Subsequent calls to rsapi_provider_get returned the cached information. When simple_get_filter is not implemented: 1. The cached data is returned, if the cache has been marked as complete and names is nil. 2. Therefore, if names is NOT nil or if the cache hasn't been marked complete, information about all resources of the given type is fetched by calling the provider's get() function. 3. The fetched information for all relevant resources is added to the cache. 4. The cache is marked complete if names is nil and simple_get_feature is not implemented. 5. Subsequent calls to rsapi_provider_get (e.g. to retrieve the current state of a resource which has pending changes) pass in a value for 'names', and therefore names.nil? is false, and the cache in point 1 above isn't returned or used. get() is therefore called again per resource to retrieve all resource information again. Removing the names.nil? checks on lines 255 and 268 allows the cache to be populated with information about all of the resources of the given type with one get() call, mark the cache as complete, and therefore allow the cache to be used in subsequent calls to rsapi_provider_get for each resource. Simple_get_filter behaviour wouldn't change, as when simple_get_filter is implemented the cache would never be marked as complete or returned: my_provider.get(context, names) would still be called every time.
b4336f6
to
476a57a
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
There may be reasons this is the way it is (I am still wrapping my head around the Puppet Resource API) - but I am wondering if we can get rid of the names.nil? checks in the rsapi_provider_get function.
Please feel free to educate me if there is something I've misunderstood or not considered.
In my testing, with simple_get_filter not implemented and operating in a Puppet Agent and Master Server environment, removing the names.nil? checks stopped get(context) from being called again for every resource which had pending changes. All resources of the given type were collected from the system with one get() call during the agent run and cached, with subsequent calls to rsapi_provider_get using the cached information. To me this seems like it would be the desired behaviour.
Additional Context
The behaviour I observed before removing the names.nil? checks:
Removing the names.nil? checks allows the cache to be populated with information about all of the resources of the given type with one get() call, mark the cache as complete, and allow the cache to be used in subsequent calls to rsapi_provider_get for each resource, whether they have pending changes or not.
Simple_get_filter behaviour shouldn't be affected by this change, as when simple_get_filter is implemented the cache would never be marked as complete or returned and my_provider.get(context, names) would still be called every time.
Testing and observations
Before the change:
After the change:
Related Issues (if any)
Related to get() call optimisation and caching.
Checklist