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

(REF) Queue - Extract better helper for batch operations #31908

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

Conversation

totten
Copy link
Member

@totten totten commented Jan 29, 2025

Background

Recall that:

Overview

hook_civicrm_queueRun_{XXX} provides basic wiring to handle a batch of data from a queue. However, support for batching is a double-edged sword for the downstream hook-implementer:

  • They have the opportunity to optimize the overall handling of the batch (e.g. to pass-through sets of data to other batch-oriented APIs).
  • They have the obligation to think in terms of batches. That's harder than thinking about items one-by-one. And if you don't need batch-optimizations, then it's unwanted effort.
    • Ex: Suppose a batch contains 10 times. When processing, 8 items succeed and 2 items fail. You need to explicitly loop through the items, assess the success/failure of each one, and update the queue-statuses for each one. And the way you handle problems should respect the queue-configuration (re: retries, aborts, failures, etc).

This PR speaks to the scenario where:

  • You want to process queue data (i.e. as an array not a Task), so
  • You implement hook_civicrm_queueRun_{XXX}, but
  • You don't want fine-grained responsibility for breaking-down the batches, toggling item-statuses, or applying an error-policy.

The implementation is based on the existing (internal) implementation of CRM_Queue_TaskRunner (which has similar goal/problem/situation). It reorganizes that code and extracts helpers so that we can reuse the logic for other (non-task) data.

Before

  • For hook_civicrm_queueRun_task, it calls out to CRM_Queue_TaskRunner.
  • If you want to handle custom array data (instead of tasks), you need a bunch of loops+conditionals. You basically need to copy CRM_Queue_TaskRunner and replace the $task->run() step.

After

  • For hook_civicrm_queueRun_task, it calls out to CRM_Queue_TaskHandler.
    • Same general behavior as before -- just a different class-name.
  • If you want handle custom array data (instead tasks), you can create another class like this:
/**
 * @service civi.queue.my_stuff
 */
class MyHandler extends AutoService implements EventSubscriberInterface {

  use CRM_Queue_BasicHandlerTrait;

  public static function getSubscribedEvents() {
    return ['&hook_civicrm_queueRun_my_stuff' => 'runBatch'];
  }

  protected function runItem($item, \CRM_Queue_Queue $queue): void {
    print_r(["Received item:" => $item));

    // If this function ends normally, then we consider the $item successful.

    // If this function encounters a recoverable error, then we check the $queue configuration
    // to decide what to do next (e.g. retry, abort, delete, etc).

    // If this function abends (PHP fatal, power-outage, etc), then any pending items in the
    // batch (incl $item and its successors) will be retried later.
  }

  // Optionally, pre-validate items before executing them.
  // protected function validateItem($item): bool {}

  // Optionally, make a loggable summary of the 
  // protected function getItemDetails($item): array  {}
}

Comments

  • This removes symbol CRM_Queue_TaskRunner. There are no references to that symbol in universe.
  • This refactors queue code. There are several existing unit-tests which cover this area (eg api\v4\Entity\QueueTest).
  • This PR was extracted from (EXP) Exploratory branch on queue updates #27860.

Copy link

civibot bot commented Jan 29, 2025

🤖 Thank you for contributing to CiviCRM! ❤️ We will need to test and review this PR. 👷

Introduction for new contributors...
  • If this is your first PR, an admin will greenlight automated testing with the command ok to test or add to whitelist.
  • A series of tests will automatically run. You can see the results at the bottom of this page (if there are any problems, it will include a link to see what went wrong).
  • A demo site will be built where anyone can try out a version of CiviCRM that includes your changes.
  • If this process needs to be repeated, an admin will issue the command test this please to rerun tests and build a new demo site.
  • Before this PR can be merged, it needs to be reviewed. Please keep in mind that reviewers are volunteers, and their response time can vary from a few hours to a few weeks depending on their availability and their knowledge of this particular part of CiviCRM.
  • A great way to speed up this process is to "trade reviews" with someone - find an open PR that you feel able to review, and leave a comment like "I'm reviewing this now, could you please review mine?" (include a link to yours). You don't have to wait for a response to get started (and you don't have to stop at one!) the more you review, the faster this process goes for everyone 😄
  • To ensure that you are credited properly in the final release notes, please add yourself to contributor-key.yml
  • For more information about contributing, see CONTRIBUTING.md.
Quick links for reviewers...

➡️ Online demo of this PR 🔗

@civibot civibot bot added the master label Jan 29, 2025
@eileenmcnaughton
Copy link
Contributor

@totten did the one I just merged make this stale?

This splits the class `CRM_Queue_TaskRunner` in two parts:

* `CRM_Queue_BasicHandler` is a queue-listener that breaks-down a batch and
  individually executes each item. It enforces queue options like
  `retry_limit=>5` and `error=>abort`
* `CRM_Queue_TaskHandler` specifically handles instances of `CRM_Queue_Task`.
…d items

This is basically the same as `releaseItems()`, but it doesn't count toward
the retry-limit.
This preserves the quirk of `CRM_Queue_Task` where callbacks are expected to
return booleans. However, for logic implemented directly on `BasicHandler`,
you can follow a simpler protocol. (Void-return on success. Exception for
error.)
@totten totten force-pushed the master-queue-handler branch from ba60215 to 777fd8f Compare January 29, 2025 03:13
@totten
Copy link
Member Author

totten commented Jan 29, 2025

@eileenmcnaughton Doh! Yes, that's what happened. I've rebased.

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