Scaling issues with batch API

+1 on the concept.

If BatchQueue is extending SystemQueue, and SystemQueue already implements DrupalQueueInterface, there's no need to re-declare that you're implementing that interface. (It doesn't hurt anything, but it is not necessary.)

BatchStaticQueue looks sufficiently generic that it shouldn't be bound to just the batch system, even by name. Heck, for PHP 5.3 it would make sense to have it use SPL's queue class internally. :-)

I can't speak for the rest of the instances, but PDOStatement is a PHP internal class while DatabaseStatementInterface is a Drupal interface. So we do have to declare that we implement that interface.

BatchStaticQueue can really be just StaticQueue, I suspect. Or MemoryQueue. Or something like that, which implies it can be used for things other than just the batch system.

Thanks for getting stuck into this. I hit the max_allowed_packet thing on the batch table doing some some huge imports (taxonomy_xml) and was shocked when I found that the queued actions I'd worked so hard to make small and atomic were being smudged together in a big serialized lump anyway!
I ended up hacking together a meta-queue thing just to make sure that I didn't put too many things into batch at once (!) By the time I'd done that, I'd almost removed any benefit of using batch in the first case (apart from the nifty progress bar!)

Using a real queue API sounds great! Good one yched!

+1 for this issue. Over the last few weeks, I did 40-50 hour monster batch jobs importing data into several sites. Any performance upgrades are much appreciated!

I spent some time reviewing this patch. It looks very nice and makes perfectly sense.

+++ includes/form.inc	13 Nov 2009 12:18:56 -0000
@@ -3073,11 +3097,28 @@ function batch_process($redirect = NULL,
+    // 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();

Don't we also have a sequences API now? Just mentioning, not sure whether it makes sense here.

+++ modules/system/system.install	13 Nov 2009 11:32:13 -0000
@@ -2358,53 +2360,9 @@ function system_update_7021() {
- * Add the queue tables.
+ * Empty update.

Please revert, but add a note that it has been moved to d7_requirements

After these, this should be ready to fly.

Thank you! Awesome work!

This scaling problem just bit us on a big batch API powered migration (a D6 site). Would love to see this committed.

I'll ask Dries to give a statement on this patch.

This question raises a bigger issue. From a functional standpoint, a method like releaseItem() should not need to be implemented by all implementors of the interface, but PHP does not implement optional methods in interfaces. And SystemQueue has no base class, but just derives from stdClass and implements DrupalQueueInterface.

If our default queue was based on a default class providing stub/default methods, implementations wouldn't need to implement each and every optional method just to complete an Interface contract. However, from #391340: Job queue API it seems that the whole Queue API was built from its initial incarnation on top of a queue Interface instead of a base class.

The problem with using a base class, of course, is that any class in PHP can only inherit from one parent, but can implement multiple interfaces, so using a base class loses generality by preventing concrete implementations from inheriting from an other class. But, given the nature of this entity, it seems it would be more natural to have concrete queues extend a base class than implement an interface.

AFAICS, there are two possibilities to avoid future problems like this one (and fix this one too)

• define the optional features as separate Interfaces, probably with those defined as extending DrupalQueueInterface, like: interface ReleasableQueueInterface extends DrupalQueueInterface { public function release(); }. Queue implementations could then implement any such derived interfaces, and their users would still have the base interface available in any implementation
• keep a rich interface definition, but define a base class implementing default methods for each method in the interface, thus offering interface implementations the ability to extend that base class and avoid having to create unimplemented stub methods. But then that would remove pretty much all interest of the interface itself.

Having both an interface definition and a common base class is not at all a bad thing. In fact it's a generally good approach, as it gives developers the maximum flexibility. The DB layer does that a lot for things like the select query builder.

Reroll, with implementation of releaseItem(). Re-running tests.

Thanks, I noticed that, too. For some reason my patching program left that out. Tests passed locally, though.

Quick - lets commit this bug fix before it needs yet another reroll.

Reroll since it would not apply cleanly anymore. I just upped the hook_update_N and fixed 2 bad hunks in form.inc

Hey, folks. I'm trying to tackle one massive patch each day before alpha. Comments body as field was tonight, I'd like to look at this one tomorrow.

The test did not complete due to a fatal error. Completion check locale.test 1195 LocaleUserLanguageFunctionalTest->testUserLanguageConfiguration()

I think i've seen this error with casey's patch today....
He retested it and it worked...

@aspilicious: FYI, the completion check is something I implemented in #560646: Fatal PHP errors don't cause tests to fail. It happens when the test method doesn't finish executing, i.e. doesn't actually mean a fatal error occurred. It could've been due to an exit() call somewhere in the test code, etc. But if retesting it worked, it's probably because a change in HEAD had fixed it since it was last tested.

So this is a massive patch, but the vast majority of it is re-organizing of test hunks. In the future, it'd be nice to separate things like that out into another patch, cos this one would've looked a hell of a lot less intimidating to review :P, and it also would've been easier to tell what was actually changed here.

I feel underqualified to adequately review the OO architecture here. It makes me a bit hesitant to commit this, since this would normally be a patch I'd ask Dries to chime in on, due to his areas of expertise. However, I see a lot of really smart people who I do trust the OO architecture reviews of, saying that this patch is good, which is a comfort. It's also nice to see the greatly expanded test coverage here.

I also cross-referenced what Batch API is doing here with the SystemQueue that was already committed to HEAD, and the two look very similar. This does indeed seem like the perfect use case for the Queue API.

And best of all, it does all of this refactoring without breaking APIs, which is double++ from me. Although a little discouraging that Moshe didn't seem to find any difference in memory usage at #281405-104: Drupal 7.x can't be installed with memory_limit=32M. Hmmm... Might only really be visible on things like massive imports, though.

At any rate, I could find nothing to complain about in here, so I went ahead and committed this to HEAD. I hope to heaven that I cvs added everything I needed to. :P

Anyone tested this patch with update.php ?
It seems that after this patch update.php hangs! It stops #563106: Cannot upgrade from Drupal 6 to Drupal 7 - meta issue

@yched This patch fixes a batch process!

Moving a {sequences} table into update_fix_d7_requirements() is a part of #563106: Cannot upgrade from Drupal 6 to Drupal 7 - meta issue

My +1 to commit it here (after chx feedback) because ugliness of #563106: Cannot upgrade from Drupal 6 to Drupal 7 - meta issue which I think to split to typos and functional fixes.

This issue much better place for both changes in #69 !!!

Committed this follow-up, thanks.

Now hopefully we can make more progress on #563106: Cannot upgrade from Drupal 6 to Drupal 7 - meta issue.

FYI, this patch caused SQLite to not install. See #684138: DatabaseConnection_sqlite->dbNextId() always returns NULL, breaking batch api, installation

#684138: DatabaseConnection_sqlite->dbNextId() always returns NULL, breaking batch api, installation is just a silly oversight in the SQLite driver, nothing to worry about here.