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

Bring back batch limit on Preload at job level #6394

Closed
MathieuLamiot opened this issue Jan 18, 2024 · 2 comments · Fixed by #6409
Closed

Bring back batch limit on Preload at job level #6394

MathieuLamiot opened this issue Jan 18, 2024 · 2 comments · Fixed by #6409
Assignees
Labels
effort: [XS] < 1 day of estimated development time
Milestone

Comments

@MathieuLamiot
Copy link
Contributor

Context

To prevent to many preload requests to be performed and hanging at the same time, especially on slow servers, we had a limit on the number of in-progress jobs, implemented here in the get_pending_jobs method.
This behavior has been changed in this PR to enforce the limit at Action Scheduler level.
The issue is that "pending async task in AS" <= "in-progress jobs". It should be equal most of the time, but the difference can be huge on slow websites. What can happen is that the async task is done (request sent) but the preload on this url is still processing (job still in-progress).
As a result, there are cases today where we can flood a website with more than 45 in-progress jobs (meaning probably more than 90 requests simultaneously because of desktop/mobile).
It also reduced the efficiency of helper plugins relying on rocket_preload_cache_pending_jobs_cron_rows_count

Expected behavior
We must never have more than rocket_preload_cache_pending_jobs_cron_rows_count in-progress jobs for the cache preload.

Acceptance Criteria

  • Manually set some jobs in-progress into the DB. Call the process_pending_jobs method. At the end of it, there should be exactly 45 pendings jobs.
  • Do the same with the rocket_preload_cache_pending_jobs_cron_rows_count filter to set the number of jobs to another number. The number of in progress job should match the filtered value.

Technical discussion
Looking at the code, I think that, by design "pending async task in AS" <= "in-progress jobs". So we could remove the check on AS queue size. But it's maybe better to keep it for safety, it probably does not cost much anyway.

@MathieuLamiot MathieuLamiot added the effort: [XS] < 1 day of estimated development time label Jan 18, 2024
@MathieuLamiot
Copy link
Contributor Author

I'm putting it in Todo, estimated to XS. It should not need a grooming as the "solution" already existed.

@MathieuLamiot
Copy link
Contributor Author

To whoever picks up this task: We should check with @CrochetFeve0251 if there was a reason to change the batch size check from DB to AS, to avoid making back and forth again.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
effort: [XS] < 1 day of estimated development time
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants