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

Add custom configuration for Action Scheduler #779

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

rmccue
Copy link
Member

@rmccue rmccue commented Jul 19, 2023

Optimises Action Scheduler to always use Cavalcade, instead of falling back to async requests. In addition, tweaks some numbers to ensure queues are processed efficiently.

Fixes #780

See also, https://github.com/woocommerce/action-scheduler/blob/trunk/docs/perf.md

@joehoyle to tweak and test these numbers further.

@rmccue rmccue requested a review from joehoyle July 19, 2023 16:15
@rmccue rmccue marked this pull request as draft July 19, 2023 16:15
@joehoyle
Copy link
Member

We pushed this to one customer, and things look to be operating as expected. I don't have data on the exact numbers, but it seems to be ok as-is.

@rmccue
Copy link
Member Author

rmccue commented Jul 27, 2023

Is it better than before?

@johnbillion
Copy link
Member

johnbillion commented Jul 27, 2023

I'd love to see the reason for the changes documented more clearly. On the rs-dms project we tweaked these settings quite a lot and made sure to document the changes clearly.

See:

@joehoyle
Copy link
Member

For documentation, I think we could elaborate on the in-code docs in https://github.com/humanmade/altis-core/pull/779/files#diff-66101e5c9a2ddcfa6f9bfa5ec8c36099c49038ceffc4182eede19db6f9406990R17-R25. @johnbillion is that along the lines you're thinking?

@joehoyle
Copy link
Member

Is it better than before?

The issue we're seeing in this case is that the fallback exists at all. The numbers on how many jobs to do at once etc is not really that important. The larger issue is that Action Scheduler will by default start making very slow HTTP requests (blocking PHP workers) if the cron jobs get slightly backed up. This PR changes that behavior so it's more binary in that sense. Yes, it's better, because we now don't use the fallback behavior and block the PHP workers doing so.

* fallbacks and maximising the Cavalcade queue.
*
* The time limit and batch size hooks are set at priority 0 to allow sites to
* easily override for their specific use case.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
* easily override for their specific use case.
* easily override for their specific use case.
*
* Specifically we disable the action_scheduler_allow_async_request_runner. This is functionality in
* Action Scheduler that will fall back to making async requests to admin-ajax.php to process the
* queued jobs. This will typically get triggered if the Cron is expected not to be working. The issue we've
* seen with this approach is when Cavalcade has a somewhat long queue, Action Scheduler switches to the
* fallback behavior, which we don't want. We don't want that because it eats up available PHP workers, where
* we prefer to just wait for Cavalcade to get to the queued jobs.
*
* We're also adjusted the limits on how many Action Scheduler Jobs will run in a single Action Schedule
* Cron Job as it's more efficient to run that way when we know how long Cavalcade jobs can run as opposed
* to the default WordPress Cron runner which likely has lower timeouts.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is preeeeettttyyyy wordy, and kinda repeats what the code says; how about:

Suggested change
* easily override for their specific use case.
* easily override for their specific use case.
*
* We disable the async task runner entirely, to avoid the loopback HTTP
* requests to admin-ajax.php. This triggers when AS detects cron not working,
* but doesn't account for the Cavalcade backlog and scaling system, and
* triggers too eagerly. Instead, we prefer a slight delay as Cavalcade scales
* up, avoiding the loopbacks.
*
* The limits are adjusted to account for the better processing power and
* limits of Cavalcade.

@joehoyle joehoyle marked this pull request as ready for review September 21, 2023 07:08
@joehoyle
Copy link
Member

@rmccue I added the above suggestion for document this further.

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.

WooCommerce jobs eat up PHP workers unnecessarily
3 participants