humanmade / Cavalcade

A better wp-cron. Horizontally scalable, works perfectly with multisite.
https://engineering.hmn.md/projects/cavalcade/
Other
515 stars 46 forks source link

`Job::get_by_site()` should hit the database when the cache is not utilised. #32

Closed dd32 closed 4 years ago

dd32 commented 7 years ago

Fixes the usage of the $include_completed and $include_failed parameters, which otherwise fail to hit the database.

rmccue commented 7 years ago

Hmm, seems like it was specifically written to do this. @tillkruss Is there a reason your PR (#26) used isset( $results ) && ! $results here?

dd32 commented 7 years ago

You're right, that at first glance it does look like that - however it also means that it only ever works if it's a cacheable call. The scenario's which do not call the cache check, do not call the db either as a result :/

dd32 commented 7 years ago

to clarify: get_by_site( $site, $include_completed = false, $include_failed = false ) = Hits Cache and then DB

get_by_site( $site, $include_completed = true, $include_failed = true ) = return array(); - no DB/Cache check. get_by_site( $site, $include_completed = true, $include_failed = false ) = return array(); - no DB/Cache check. get_by_site( $site, $include_completed = false, $include_failed = true ) = return array(); - no DB/Cache check.

roborourke commented 4 years ago

@dd32 sorry for the massive delay here. We've just updated cavalcade to use the preflight filters that landed in WP 5.1, this also adds a get_jobs_by_query() method which uses the query args to generate a hash key. get_jobs_by_site() now uses this under the hood.

It should fix the problem so I'm going to close this out for now but feel free to comment / reopen if you find any problems.