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

Queue API - Expose option for delayed tasks ($options['release_time']) #24395

Merged
merged 1 commit into from
Aug 27, 2022

Conversation

totten
Copy link
Member

@totten totten commented Aug 26, 2022

Overview

CRM_Queue_Queue_*::createItem() allows you to create new tasks for separate/parallel execution. This patch adds an option for marking new tasks as delayed (ie they will execute at some specific time in the future).

This is an off-shoot from discussion on https://lab.civicrm.org/documentation/docs/dev/-/merge_requests/992 (@eileenmcnaughton @jaapjansma @ErikHommel).

Before

  • When creating a task with CRM_Queue_Queue_*::createItem(), you cannot request delayed execution explicitly.
  • However, the underlying system is amenable. (Delays are already used for locking tasks and for retrying failed tasks.)
  • Some extensions already use the SQL-backed queue for delayed tasks, and there's a public recipe for doing this. But it requires one to directly manipulate the queue via SQL/DAO.

After

All core queue types (Sql, SqlParallel, Memory) all you to create a delayed task:

Civi::queue('todos')->createItem(
  new CRM_Queue_Task($callback, $args),
  ['release_time' => strtotime('+3 hours')]
);

@civibot
Copy link

civibot bot commented Aug 26, 2022

(Standard links)

*/
public function createItem($data, $options = []) {
$dao = new CRM_Queue_DAO_QueueItem();
$dao->queue_name = $this->getName();
$dao->submit_time = CRM_Utils_Time::getTime('YmdHis');
$dao->data = serialize($data);
$dao->weight = CRM_Utils_Array::value('weight', $options, 0);
if (isset($options['release_time'])) {
$dao->release_time = date('Y-m-d H:i:s', $options['release_time']);
Copy link
Member Author

@totten totten Aug 27, 2022

Choose a reason for hiding this comment

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

Fun fact (and pre-existing issue): civicrm_queue_item.release_time uses DATETIME. This is ambiguous - there is no clear definition of the storage-time-zone.

Of course, AFAIK no one has been complaining about that. I suppose a lot of queue-stuff is done by background workers which (for a given deployment) will have consistent/mutually-agreeable TZ settings. (Or maybe it's just hard to notice - how many people will recognize when the lock-release of a failed background task is wrong by a couple hours?)

The calculation here should be as good as any existing calculations on this column (as performed in this recipe or in core) -- ie it's OK for simple deployments with relatively consistent timezones, but it's highly suspect for multi-time-zone usage.

All of that really should be a separate issue/bug. But it's a bit relevant in how it influences the interface of $options['release_time']. In this patch:

  • $options['release_time'] is not string (Y-m-d H:i:s) because that is ambiguous. (It is unclear whether the timezone of Y-m-d H:i:s should be the current-user-TZ, the queue-worker-TZ, the server-TZ, or GMT. The behavior that would apply at this moment is not the behavior that would be generally good.)
  • $options['release_time'] is int (seconds since epoch) because that is unambiguous.

Copy link
Contributor

Choose a reason for hiding this comment

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

yeah - the column should be a timestamp column - any system-used date column should be

@eileenmcnaughton eileenmcnaughton merged commit 27d5300 into civicrm:master Aug 27, 2022
@totten totten deleted the master-release-time branch August 27, 2022 05:38
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.

2 participants