Skip to content

Conversation

@Denitz
Copy link
Contributor

@Denitz Denitz commented Feb 23, 2022

Summary of Changes

PlgSystemSchedulerunner::injectLazyJS() checks the presence of due tasks via weird loading of items list with order by title DESC using the admin model:

SELECT a.id, a.asset_id, a.title, a.type, a.execution_rules, a.state, a.last_exit_code, a.locked, a.last_execution, a.next_execution, a.times_executed, a.times_failed, a.ordering, a.note
FROM `jos_scheduler_tasks` AS `a`
WHERE `a`.`state` = :state AND `a`.`next_execution` <= :now
ORDER BY `a`.`title` desc

Assuming that we are not loading the data but only counting the tasks which should be executed, we should not retrieve the full list of items, but only count them.

We have a second weird query to check if there are any locked tasks, the same invalid behavior occurs again:

SELECT a.id, a.asset_id, a.title, a.type, a.execution_rules, a.state, a.last_exit_code, a.locked, a.last_execution, a.next_execution, a.times_executed, a.times_failed, a.ordering, a.note
FROM `#__scheduler_tasks` AS `a`
WHERE `a`.`state` = :state AND `a`.`locked` IS NOT NULL
ORDER BY `a`.`title` desc

These two queries can be optimized into single efficient query which loads the number of due tasks + the number of locked tasks:

SELECT SUM(CASE WHEN `a`.`next_execution` <= :now THEN 1 ELSE 0 END) AS due_count, SUM(CASE WHEN `a`.`locked` IS NULL THEN 0 ELSE 1 END) AS locked_count
FROM `#__scheduler_tasks` AS `a`
WHERE `a`.`state` = :state

Using backend list model is not efficient, better compose the query in the plugin.

The result of single query is checked and the plugin is not adding JS if we don't have due tasks OR we have locked tasks.

Testing Instructions

Test scheduled tasks

Actual result BEFORE applying this Pull Request

See two queries for #__scheduler_tasks table.

Expected result AFTER applying this Pull Request

See one query for #__scheduler_tasks table.
com_scheduler works as before.

Documentation Changes Required

No.

@Denitz Denitz requested a review from richard67 February 23, 2022 15:49
Copy link
Member

@richard67 richard67 left a comment

Choose a reason for hiding this comment

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

Looks ok to me by review.

@ditsuke
Copy link
Contributor

ditsuke commented Mar 26, 2022

It was a deliberate decision to avoid any queries that bypass the model. That said I really like this, but could we instead add this as a method in the Scheduler model which could be then accessed through the Scheduler API class? That way we could avoid any localized logic in the plugin and also increase the useful API surface of the Scheduler which could be useful to other extensions. Thank you for working on this!

@Quy Quy added Updates Requested Indicates that this pull request needs an update from the author and should not be tested. and removed Information Required labels Jun 5, 2022
@Quy Quy removed the Updates Requested Indicates that this pull request needs an update from the author and should not be tested. label Jun 8, 2022
@HLeithner HLeithner changed the base branch from 4.1-dev to 4.2-dev June 27, 2022 13:05
@HLeithner
Copy link
Member

This pull request has automatically rebased to 4.2-dev.

@Denitz Denitz changed the base branch from 4.2-dev to 4.3-dev March 24, 2023 20:59
@Hackwar Hackwar added the bug label Apr 6, 2023
@obuisard obuisard changed the title Optimize scheduler running plugin [4.4] Optimize scheduler running plugin Jun 2, 2023
@laoneo laoneo changed the base branch from 4.3-dev to 4.4-dev June 9, 2023 06:17
@laoneo
Copy link
Member

laoneo commented Jun 9, 2023

Can you fix the conflict?

@MacJoom
Copy link
Contributor

MacJoom commented Aug 9, 2023

@Denitz - thank you for your work - what about ditsukes comment, moving the query to the model - would be nice.

@Denitz
Copy link
Contributor Author

Denitz commented Aug 10, 2023

@MacJoom The query was moved to the model a year ago.

@MacJoom
Copy link
Contributor

MacJoom commented Aug 10, 2023

Sorry i missed that one

@MacJoom MacJoom enabled auto-merge (squash) September 11, 2023 19:37
@MacJoom MacJoom disabled auto-merge September 11, 2023 19:37
@MacJoom MacJoom merged commit 2875f27 into joomla:4.4-dev Sep 11, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

10 participants