-
Notifications
You must be signed in to change notification settings - Fork 213
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
Limit numbers of pending RUCSS jobs to avoid being late on fetching the result and hence getting a 400 Bad Json #6213
Comments
From @wordpressfan: (Slack)
|
@wordpressfan I agree with the approach. However, it should not impact the 408 retry mechanism here
Where MAGIC_CONSTANT is to be defined (> 100 not to block the default batches, and not too big to avoid the problem we currently have. Maybe 300?) @piotrbak, what do you think in terms of product impact? |
@wordpressfan @vmanthos what was discussed during the daily is that we could replace I would suggest n to be ~3 (it has to be more than 2 and not to big...)
(And please, let's not hard code 3 and 100 but use (shared) constants instead? ; maybe put the inner apply_filter in a method as we use it at different places in the codebase) |
I'm just worried about hammering the DB with the COUNT query every time a job is added 🤔 |
Yes it's a small query and the table is indexed well so I believe we are fine but this is a good concern, thanks for discussing it. The big picture that I was thinking of last night is:
This will give us the control of how many jobs are created at once and not to miss any job but also will add a layer of complexity. |
Reproduce the issueYes I can see the issue in one of our customer's site. Root causeJobs are kept in plugin's database even when they are removed from SaaS side so every check status after that time will fail because SaaS at this point returns empty response. Scope a solutionAs We mentioned above we need to add a new status
And change the status to
Effort[M] or [L] @CrochetFeve0251 @wp-media/php to review plz. |
Looks good overall. Some comments:
Point 2: Same here, let's just remove the job_id argument. Point 3 & 4:
in the Point 5: Make sure to add Point 9: We can use this limit for the number of "to_submit" jobs: Point 10: Do you foresee an issue in using a for loop to go through the (up to) max_pending_jobs RUL and making the API calls? for fetching the results, we use async tasks for each API call. Is there a reason for this approach? |
@wordpressfan The grooming looks good but there is one point I am not sure about is the Another thing this issue is working on the exact same methods with different AC. I guess we need some communication to not end up with monster conflicts. |
|
After a discussion with @MathieuLamiot and @CrochetFeve0251
|
@CrochetFeve0251 Can you provide a documentation for the new submission system and the related filters, so that support can learn how to use them? Thanks! |
Context
Several users with an high number of pages on their websites reported issues with RUCSS feature, resulting in "400: Bad JSON".
Some investigations showed that, when starting the process of RUCSS optimization on the plugin for the whole website, the first URLs were correctly optimized but at some point, all jobs started to return 400: Bad JSON. This seem linked to the fact that the website does not manage to fetch all jobs in the expected schedule: too many actions to fecth results are planned and a good part of them take place to late, when the job is stale on the server side.
Expected behavior
The plugin should not send new jobs if it is not capable of fetching them back in the appropriate time window. Therefore, we should keep the number of pending jobs under control:
The maximum number of pending + in progress job must be configurable with a filter for instance. With a special value (for instance 0?) the configuration means that the limit must be disable (come back to the current state).
Acceptance Criteria
The text was updated successfully, but these errors were encountered: