Make batch a wrapper around the queue system

This isn't a feature request.

May or may not be related but #1548286: API for handling unique queue items was opened a while ago since update module is using it's own cache system to track the status of queue items. If we need an API for "what's in the queue at the moment" then that might end up being the same thing. The idea there was that we have a separate layer above the queue API which tracks what's in it, since you can't rely on the queue storage itself to do that (and it's not reasonable to).

Don't improve the API just convert it to queue... to begin with. Then you can re-use the queue reporter/runner screen to run other queues.

This would be helpful for Drush as well as we have custom batch api processor AND a queue-run command. I'd love to just have queue-run.

Beside queues..

One of the IMHO most annoying things in the batch API is,
you can alter batches - but that's pretty useless because the id is not available yet.

    // The batch is now completely built. Allow other modules to make changes
    // to the batch so that it is easier to reuse batch processes in other
    // environments.
    drupal_alter('batch', $batch);

    // Assign an arbitrary id: don't rely on a serial column in the 'batch'
    // table, since non-progressive batches skip database storage completely.
    $batch['id'] = db_next_id();

this makes it really hard to associate custom data to batches.
Could we just swap these two lines?
that would be very little effort, but could have pretty high impact for some of us

Interested parties should take a look at this. It's showing fairly well where I am headed. No conversions yet but the workflow is that you create a batch with create(), add items to it with createItem() then redirect to the user batch runner page.

Sorry, I dunno how this moved to catch.

This allows the installer to use this API.

@chx Thanks for moving this issue forward.

Let me know when it is appropriate to check/add-in the appropriate documentation so that this patch can be committed.

I really like where this is going. Batch API should absolutely be backed by Queue API. I know this isn't done, but partial review below.

+++ b/core/lib/Drupal/Core/Batch/BatchManager.php
@@ -0,0 +1,141 @@
+  protected function getUid($uid) {
+    return $uid ?: $GLOBALS['user']->uid;
+  }

This method doesn't even make sense...

+++ b/core/lib/Drupal/Core/Batch/BatchManager.php
@@ -0,0 +1,141 @@
+    $queue = queue($queue_name, TRUE);

This still needs to get converted to something injected, like KVFactory.

+++ b/core/lib/Drupal/Core/Batch/BatchManager.php
@@ -0,0 +1,141 @@
+  /**
+   * @param $queue
+   *   The queue object returned by create().
+   * @param $data
+   *   The data for this item.
+   * @param $callback
+   *   The callback handling the data.
+   * @return bool
+   *   TRUE on success.
+   *
+   * @see \Drupal\Core\Queue\ReliableQueueInterface::createItem().
+   */
+  public function createItem(ReliableQueueInterface $queue, $data, $callback) {
+    $item = array(
+      'data' => $data,
+      'callback' => $callback,
+    );
+    return $queue->createItem($item);
+  }

The intent here is to provide a matching interface so that you can interact with BatchAPI the same way you do Queue API, right? Without a full docblock I'm not certain of that, and the queue parameter makes me confused.

+++ b/core/lib/Drupal/Core/Batch/BatchManager.php
@@ -0,0 +1,141 @@
+  public function jsPage($uid) {
+    $total = intval($_GET['total']);
+    $batches = $this->getBatches($uid);
+    $current_total = 0;
+    $items = array();
+    foreach ($batches as $name => $description) {
+      $queue = queue($name);
+      $current_total += queue($name)->numberOfItems();
+      $items[] = $description;
+    }
+    if (isset($queue)) {
+      $item = $queue->claimItem();
+      call_user_func($item['callback'], $item['data']);
+    }
+    $percentage = 1 - (($current_total - 1) / $total);
+    return new JsonResponse(array(
+      'status' => TRUE,
+      'percentage' => (string) $percentage,
+      'message' => theme('item_list', array('items' => $items)),
+    ));
+  }

BatchManager itself shouldn't know about responses. It should know about managing and running jobs. Http-awareness doesn't belong in BatchManager, but in the controller that calls to it (or the Drush command that does so, or Console command, or whatever).

We need the page callbacks somewhere reusable because user module wants to run them access controlled and the installer can't have access control. I moved the actual run into a method.

@ Lars Toomre, when the "Needs architecture review" tag disappears.

Thanks @chx.

Talked this over with chx in IRC. The startPage(), jsPage(), and finishPage() methods need to move out of BatchManager. chx will follow up with a patch that moves them all to a separate class (I recommend Drupal\Core\Batch\Controllers.php, or possibly Drupal\Core\Batch\Controllers\Something.php) setup as controller methods, possibly with some shared protected methods if appropriate. We'll then provide 2-3 page callback functions in system.module or user.module or whatever that are just trivial wrappers around those methods.

When we're able (in Slush) to convert those callbacks to new-style routes, we'll simply drop those wrappers and map to the controller methods directly.

That keeps the BatchManager service independent of page callback/controller concerns, as it should be.

(There's still the question of that uid, but we'll deal with that. :-) )

Big +1 on the task, of course. Will try to review properly when I get to the Ghent sprint this week end.

For now, AFAICT, current patch does not support multipass operations ? (those that return a "not finished, call me again" status and thus should not be dequeued after being run)

yched: I think the intent is that you'd break those up before hand, not during, the way you do with Queue API. So processing 10000 nodes would be a matter of enqueuing 10000 nodes, then just churning the queue one at a time. Vis, the "not done, call me again" logic becomes unnecessary.

@Crell : thus remove multipass upgrades ? Not sure that works.

Also
- that's 100.000 "add item" inserts instead of just one currently, not the same performance-wise.
- you have no guarantee that the node you queued still exists - or is still eligible for the task - by the time its corresponding queued item gets effectively processed, so that's more possibilities for batched jobs to be badly coded. Increases race condition windows by an undefinite amount of time.

I deliberately do not support "not finished, call me again" jobs. It would entirely be possible as there is a releaseItem and we could do that if we wanted. But I don't want to. It makes it impossible to see really what's happening and how much work is left and it makes parallel processing impossible. Noone processes 1 node a time. We process them 50-100 at a time so we are talking of like 1000 inserts. That's a few seconds on a very weak and slow system, a fraction of a second on anything that can handle 100 000 nodes anyways :)

Note the node mass update bis still not finished because the feedback is inferior, I will see what I can do to help that -- likely use K-V for feedback. Also you need to visit the user/%/batch page manually, I need to figure how to tell the API to execute stuff immediately.

Another TODO is cron. And tests.

In most cases, the "please call me again" callback needs to know the ids anyway (unless it's just deleting all items, for example).

So in VBO's case, the queue needs to be filled manually (and that filling might be done in a batch too, if we're selecting multiple views pages).
We split the ids in groups (of 10, for example) so that we have less inserts, and we can bulk entity load the ids.
The ids for which we couldn't load the matching entities get skipped.

In most cases, the "please call me again" callback needs to know the ids anyway

Well, in most multipass ops, you dynamically select the next N nodes to process in the iteration by a new query.
"SELECT 50 nodes WHERE nid > (last nid processed) AND (the relevant condition).

That's not the typical use case for VBO, because VBO processes as arbitrary list of user selected ids, but I'd argue that VBO is the exception, and the above is much more frequent across existing cases of multipass batch ops / update funcs.

Understood, thanks for clarifying.

More generally, regarding ditching support for multipass - well, that's funny since Batch API started as a generalization of the one-off mechanism that was used by update.php until D5, with a critical use case for multipass updates.

Without support for multipass, how do you write a hook_update_N() that does something on all nodes on a site ?

Like this:

$chunk = 100;
$max = db_query('SELECT MAX(nid) FROM {node}')->fetchField();
$batch_manager = drupal_container()->get('batch.manager')
$queue = $batch_manager->create('Batch all the nodes');
for ($i = 0; $i < $max; $i+= $chunk) {
  $batch_manager->createItem($queue, array(':min' => $i, ':max' => $i + $chunk - 1), 'allnodecallback');
}
function allnodecallback($bounds) {
  $nids = db_query('SELECT nid FROM {node} WHERE nid BETWEEN :min AND :max AND whateverelse', $bounds)->fetchCol();
  $nodes = node_load_multiple($nids);
}

if you have holes in the node table, then a couple of callbacks will finish faster. Never heard anyone complaining about fast.

@chx: Will the update hooks be called by this as well or how will that work? Because that would require an ordered queue implementation that allows to append and prepend queue items (or have some other system, like a sub-queue, which is processed before continuing with the next update function).

Could you give me your use case where you need to append/prepend? BTW. ReliableQueueInterface is already ordered (it's a FIFO).

@chx: update functions. Assuming they are executing as a batch-queue-thingie themself, the queue items they add need to be executed before the next update function runs because that might depend on it.

We can add an operation which move an item to the end of the FIFO. Now... the update runner checks whether the updates it needs are finished yet (it can store in K-V what updates are done). If node_update_8000 needs to run a sub-queue then it does just queues more jobs and then adds a new job which is like node_update_8000_finished. If anything depend on node_update_8000 those wont be able to run and they will use the "move to the back operation".

Edit: runOne will need a loop to make sure one successful job actually happens.

+++ b/core/modules/user/user.pages.inc
@@ -424,3 +426,14 @@ function user_page() {
+function user_batch_page(User $account) {
+  $batch_controllers = drupal_container()->get('batch.controllers');
+  $uid = $account->id();
+  $op = isset($_GET['op']) ? $_GET['op'] : 'start';
+  $method = $op . 'Page';
+  if (method_exists($batch_controllers, $method)) {
+    return $batch_controllers->$method($uid);
+  }
+  throw new NotFoundHttpException();
+}

I'm unclear why this is still a single page callback. Shouldn't it be split into 3, matching the 3 controllers in the class?

Discussed with chx on IRC:
- Batch API code definitely suck big time, can really use a rewrite by smart people
- Agreed that multipass don't really fit with usual tools and concepts about queueing jobs. Most real-life use cases for batch API consist of one single multipass op, so that would mean running 1-item queues, which makes little sense.

So I'm fine with the approach.

In response to the blogpost, yes I'm fine with this as well. The described approach translates to "Instead of trying to calculate all dependencies upfront, we just start somewhere and move everything whose dependencies aren't yet met back at the end of the queue". That does sound a bit scary, but could work. I do have two questions remaining:

- How do we detect an endless loop/circular dependencies?
- How do we actually define dependencies?

Side note regarding the example code in #24 :
That one is wrong, because if new nodes are created after the job is queued, some of them (but possibly not all if the number of new nodes exceeds the $chunk size) will be processed as well -> underministic behavior.

This probably wouldn't be a problem in updates, because you "usually" try to stop all activity on the site during updates (even though: nodes created by cron tasks...), but wouldn't fit for "regular" (non update) jobs.

Whether nodes that didn't exist when the job was created should be treated or not depends on the specific task at hand. But at any rate the code above would sit in between.

This does not mean that the new approach is flawed, it just means that coding asynchronous tasks right is inherently *hard*, and depends on your use case - whether using multipass or separate jobs pushed upfront, and thus there's only so much the API can ease. Coding massive processing tasks won't become magically easier with the new API :-) (just saying this to stand up for poor old Batch API's DX...)

Re #32/#24 we can change to min($max, $i + $chunk - 1) easily. That is solved.

How do we define dependencies? Well, update has a mechanism for cross module dependencies. And we will simply add that hook_module_{N+1] depends on hook_module_N to make sure things remain sane, update is scary enough without trying to think it through which update can run after the other. As for circular dependencies, don't we have a Graph component already? Cultivate a dependency graph in K-V and add the capability to Graph to detect a circle, it might have that already, actually.

@chx: of course that's easily solved :-). My point was : whether old or new API, writing your batched jobs correctly will remain a subtle task depending on the exact nature of your task...

#19 did not apply any longer, reroll:

So tasks to do

1. Implement #24 for the existing long running updates.
2. Add a dependency system: store dependencies in the keyvalue storage, use the exisiting Graph component perhaps to make sure you have all of them. Careful with circular dependencies.
3. Use the new moveToTheEnd operation to move jobs which have dependencies to run.
4. Add implicit dependencies of hook_update_{N+1} on hook_update_N

This patch converts the node module and the update.php stuff to the "new batch api".

As some of this queue handling is very repetitive, I added a new class called "Batch" (basically wraps BatchManager calls) which can be used to create a new batch:

use Drupal\Core\Batch\Batch;
// Create the queue and save batch information.
$batch = new Batch('Node mass update', $updates, $options);
// Add an operation to the queue.
$batch->addOperation('_node_mass_update_batch_process', array_pop($nids));
// Add multiple operations to the queue. (Pass all the $nids to the callback in packages of 10)
$batch->addOperations('_node_mass_update_batch_process', $nids, 10);
// Just a redirect to the user batch page.
$batch->start();

Also batch some batch $options have been revived such as:
- file: the file to include for the operations
- finished: the finished callback
- redirect: the path to redirect when finished
- redirect_options: options to pass to url() on redirect

Probably my largest core patch ever and still a lot to do, so bear with me ;) hope it helps

Applies well for me on a fresh cloned 8.x, maybe bot is a bit overloaded again...

Reuploading the same patch.

Fixing tag

#40: 1797526_37.patch queued for re-testing.

patch needs re-roll

// Create the queue and save batch information.
$batch = new Batch('Node mass update', $updates, $options);
// Add an operation to the queue.
$batch->addOperation('_node_mass_update_batch_process', array_pop($nids));

To what extent are batch operation callbacks reused?
I'm not sure about core, but certainly where I've seen batch processes in contrib, the operation and finished callbacks are only used for that one batch.

Therefore, I wonder whether a nicer DX would be to define a batch operation as a single class, with methods along these lines:

- batchInfo(): returns an array in the same sort of format as currently passed to batch_set()
- operationFoo()
- operationBar()
- finished()

You'd then do batch_set('MyBatchClass'). The advantages would be:

- all your related batch code is in one place, rather than split between the batch info array in the form submit handler and multiple callbacks
- no need to think about file inclusion if your batch callbacks are in .inc files

Moving back to 8.x for two reasons:

1. We're running into issues with configuration that could use a more flexible batch API, for example #2198429: Make deleted fields work with config synch

2. There's no reason this couldn't go in parallel to batch API in a minor release of 8.x, and we then remove the old batch API in Drupal 9.

Also this is a classic example of why 'feature freeze' is a horrible and wrong concept - lots of features got committed after this was moved to 9.x that were much less important than this issue.

Too late for D8?

This should probably go into 8.2.x-dev, but I don't see that option.

Rerolled the patch for 8.2.x

Rerolled the patch for 8.2.x

I am able to apply patch successfully, test pass also, But why i am getting CI error? can any one point me what is my mistake?

+++ b/core/modules/node/node.admin.inc
@@ -31,8 +31,23 @@
 function node_mass_update(array $nodes, array $updates, $langcode = NULL, $load = FALSE, $revisions = FALSE) {
+function node_mass_update($nids, $updates) {

@@ -62,6 +77,7 @@ function node_mass_update(array $nodes, array $updates, $langcode = NULL, $load
   }
+  }
 }

syntax error, unexpected end of file in ./core/modules/node/node.admin.inc on line 143

You're leaving the old function in place.

Yeah this is not a patch which just needs to be rerolled.

Discussed this a bit in irc with dawehner and neclimdul

- I think we should look at adding a new API/subsystem, parallel to batch API, rather than a refactor
- basing it on queue would be great, although we probably need more than just a UI queue runner/progress reporter for that to work
- usages can eventually be converted to the new API
- all of old batch API can be deprecated for 9.x

The biggest challenge I can see is that a batch is a single set of operations, that is started, processed and finished by one user.

Whereas queues are added to from anywhere and processed by whatever queue runner picks them up.

So providing a browser-based queue runner is not that bad, but providing the same end-to-end UI where you start something, process it, and see it through to completion in one place would be trickier - i.e. what if two users do something with the same queue at the same time?

But we might be able to change expectations so that this doesn't matter as much - for example not making the queue running a blocking operation, but show it as a status message instead.

#64: That seems like a solvable problem to me, and the queue would certainly offer robustness benefits. Building a BatchTNG(tm) and migrating to it sounds like a safer plan at this point, I agree.

1) we can use a random ID or some other hash as part of the queue name for each BatchTNG run. That should keep 2 users from colliding.

2) The other big difference is that a queue generally doesn't know its size-remaining, but for batch messages we'd need to. That seems like something that could be tracked in parallel via state API, and would stay in sync unless something fataled in which case it's an off-by-one race condition. That also seems like a solvable problem.

1) we can use a random ID or some other hash as part of the queue name for each BatchTNG run. That should keep 2 users from colliding.

Yeah it seems to be that we are maybe able to store the actual instance in tempstore or so.

Here is just a really quick idea how this could be organized:

class BatchQueue {
  // Has a reference to a actual queue implementation stored underneath.
  // The batchQueue gets passed along to individual batch callbacks, so they can add new items.

  public function addBatchItem(BatchItem $item) {
    // Keeps track of total amount of items.
  }

  public function getTotalCount() { }

  public function getProcessedCount() {
    // total - amount of items left in the queue.
  }

}

class BatchItem {
  public function create($title, $callback, array $arguments = []) {}
}

class BatchFactory {

  public function createBatchQueue(): BatchQueue {}

  public function loadBatchQueue($id): BatchQueue {}
  
  public function startInteractive(BatchQueue $queue): RedirectResponse {}

  public function processInteractive(BatchQueue $queue): Response {}

  public function processNonInteractive(BatchQueue $queue) {}

}

Yeah it seems to be that we are maybe able to store the actual instance in tempstore or so.

Something like that could work, but we should leave open more advanced use-cases like the following:

- UI reporting-only with an 'instant' background queue runner like https://www.drupal.org/project/waiting_queue

- non-blocking queue runner in messages area or similar, which might be fine with a generic queue

As well as modernising batch API, this issue opens up the possibility to do more things in queues - for example we could queue all e-mails and send them in a non-blocking UI queue runner by default (currently they're blocking i/o in POST requests by default, as well as messing up render contexts), then there are long standing issues like #89181: Use queue API for node and comment, user, node multiple deletes.

@dawehner, I like this variant of batch implementation.

I happened to be looking at batch API code last week -- it's already a wrapper around the queue system in as much as when you start a batch, it puts all the operations into a queue.

Does that make this issue outdated, or it is about furthering the use of the queue somehow? If the latter, the issue summary needs to explain in more detail.

Re: [23:26:51] cweagans: ie. Amazon SQS is utterly unusable for this.

SQS is actually usable: if you don't call DeleteMessage, for example, after the VisibilityTimeout expires, the message becomes visible again and can be take by another worker. Alternatively, if you need a longer delay than the max of VisibilityTimeout, you can still call DeleteMessage and produce it back with another CreateMessage call, copying the origin metadata from the received message to the newly produced one.

Is this issue still valid?