#75 WP 5.1 Pre-flight filters

[roborourke]

Forked from #84

Fixes #75 Fixes #80

[roborourke]

@peterwilsoncc forked your branch so we could get the linter errors and tests working again. This is awesome work bud

[peterwilsoncc]

Coolio.

I think this will need some pre-release testing on the hm site so the servers team can monitor the sql queries.

At the moment I think they’ll explode as caching isn’t persistent and there will be a few unindexed searches.

[peterwilsoncc]

I think this will need some pre-release testing on the hm site

Actually, I think the H2 network might be better due to the number of notifications, it will probably give you a better idea.

You'll also probably want to prevent WP from loading the cron array during bootstrapping (I think removing the option from all options will do it) to avoid the database query the precommit hooks are intended to avoid.

[roborourke]

@peterwilsoncc ah yeah, I saw that comment but thought it meant that's the same level of caching as we have currently.

I'll look into the alloptions thing, not sure to do that off the top of my head. I'll try this out on Altis locally.

[rmccue]

The querying shouldn’t be an issue, since it’s just part of alloptions anyway (which is a single query for all of them, but which we handle in the cache anyway), right? Or is there another query here?

[peterwilsoncc]

@rmccue

After detecting the option in alloptions a get_option()` call is made which misses the cache due to the preflight filter Cavalcade hooks in to.

As the cavalcade-jobs cache is non-persistent, a new query is trigger on each load. It's then missed on subsequent calls to get_jobs_by_query() as they use a different where clause.

Random notes:

• 86 includes a proposal for an updated DB schema to improve indexing

• Persistent cache using the wp_cache_get_last_changed() pattern and deleting the key when a job is added/run. Ugh, ugly.
• Finish query class in #86 but good.

[roborourke]

@rmccue made a couple of changes if you wouldn't mind doing a quick check through. You'll have to download the core cron tests manually but you can run the tests in the Altis environment using composer dev-tools phpunit -- vendor/humanmade/cavalcade/tests/tests/<file>.php --exclude-group 32656

[roborourke]

@rmccue looking at the cavalcade-jobs group I'm not sure I understand why it's not using a persistent cache? Given the implementation in get_by_site() there are only 4 different sets of results for each site:

• status: waiting, running
• status: waiting, running, completed
• status: waiting, running, failed
• status: waiting, running, completed, failed

It'd be easy enough to clear those 4 keys on any CUD operation.

[roborourke]

Thanks @peterwilsoncc, I've simplified it back down to the core WP approach to cache invalidation. I'd still like to understand why we intentionally use a non persistent cache though.

[peterwilsoncc]

Thanks, I'll review and test on the weekend.

I've opened #93 to discuss whether caching should be persistent and if it needs to be global.

[rmccue]

looking at the cavalcade-jobs group I'm not sure I understand why it's not using a persistent cache?

When it's used and updated in Cavalcade Runner, it doesn't have proper access to the object cache; in particular, we don't know (IIRC) the cache key generation algorithm. Because of this, the cache can't be authoritative.

On my todo list to properly review this!

[roborourke]

Thanks for the explanation @rmccue. Seems kinda wild that the cache key generation isn't the same, that's a shame. I hadn't considered the runner environment.

I think to move this forward we leave the cache as non-persistent and the db update in #95 will give us a performance boost in a different way.

[rmccue]

Seems kinda wild that the cache key generation isn't the same, that's a shame. I hadn't considered the runner environment.

It's an internal implementation detail of the object cache, but we're not loading the cache at all in the runner, so we don't know how to use the cache (i.e. how does the runner know if you're using memcache or Redis?). We could potentially load it in, but we're then tying the runner heavily into how WP works; also, we're likely to hit memory errors due to the OC's in-memory cache.

[roborourke]

@peterwilsoncc @rmccue I also noticed in another place we're respecting any filters that run before ours but with this update we're not checking the value of $pre as passed in. I'm thinking we probably should allow that.

See this branch for what I mean: https://github.com/humanmade/Cavalcade/compare/75-wp51-filters-fork...respect-the-filtery?expand=1

[peterwilsoncc]

@roborourke Yeah, let's. I'm inclined to keep the hooks on 10 but we should allow for other plugins.