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.

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

Retrieve job from cache in Job::get() #44

Closed
wants to merge 2 commits into from

Conversation

igmoweb
Copy link

@igmoweb igmoweb commented Jun 30, 2017

No description provided.

@joehoyle
Copy link
Member

joehoyle commented Jul 4, 2017

The reason we don't have object caching here is because the Cavalcade Runner can change the database without memcached knowing about it. It doesn't (currently) run with the object-cache.php loaded (it's not a WordPress process). It's probably / maybe technically possible to do that though, but unsure if worth it.

As it stands right now, merging this would break a lot of things, @rmccue any idea if we want to try improve the cachability of the cavalcade db queries?

@rmccue
Copy link
Member

rmccue commented Jul 5, 2017

cavalcade-jobs is a non-persistent cache (see discussion on #26), so this would only fetch from the cache if we're getting the same job repeatedly in the same process. It won't break anything, but I'm also not sure it'll have a huge effect on performance.

@igmoweb Without the persistent caching, is it worth doing this?

@igmoweb
Copy link
Author

igmoweb commented Jul 5, 2017

I missed that it was non-persistent one.

Definitely, it's not a big performance improvement but I still made the PR for the sake of consistency (jobs are cached so why not to get the jobs from cache on this function?) and because I was not sure how many times the function can be called in a single load. Now that I know that the cache key is non-persistent, the query is done at least once but could improve subsequent calls. Still, not a big deal and could not worth the risk unless we see cases where the function is being called many times.

@igmoweb igmoweb closed this Sep 29, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants