Improve cron handling (don't update all users)

Hey everyone,

I love this module, but I've also had quite a few problems using Mailchimp with cron. It causes cron to fail on a pretty regular basis. At the same time, I can't update Mailchimp immediately when a user's account is changed because if Mailchimp's API is down (which has happened repeatedly), it causes every account update page on my site to hang indefinitely.

We've got 11,000 users and the way Mailchimp's cron seems to work, it has to go through and update all 11,000 users (including users whose e-mails have already been bouncing repeatedly).

How about adding a column in the database that tracks only those accounts that have been updated or created since the last cron run. Once cron ran, this column would then be reset. Then, we would only need to update Mailchimp during cron with any changed accounts (thereby reducing the cron burden for sites with a lot of users).

What do you think, LouBabe and ronin?

Cheers,
Ben

Similar issue at #707676: Cron Run Batch Processing (now marked dupe)

Suggestions -

1. mailchimp.module could add code to mailchimp_cron() to batch process users, cycling through the complete users table over many cron runs.

2. mailchimp.module could add code to mailchimp_user() to track users which have updated their user profile / settings, and only update those users on cron update.

I think (2) is a better approach, but interested in other suggested methods or comments on these.

Yeah, I agree that option 2 looks good.

As you say on hook_user we can set a 0/1 value to user->data['mailchimp_needs_update'] or something, depending on whether or not mailchimp settings were changed. Then check that value on cron.
It would have to be done on the insert & submit ops.

The value would then be reset to 0 during the cron run when the user is processed.

That way all updated users don't have to be done, only those who updated their mailchimp settings.

That may not help a site with 250K users much, though; they'd still need to user_load() each user for each cron run just to inspect $account->data and then ignore 249,478 of the users, wouldn't they?

I think we need a table for mailchimp_user_pendingupdate or something here.

Yeah good point, a separate table would be far better.

OK, maybe not a separate table ... I don't see that it adds much over storing as an array of UIDs in a variable_set() (except if the array is particularly huge ... is 40K too much?)

Some notes on looking at this:

• Mailchimp recommend not tapping more than 5-10K in a single $q->listBatchSubscribe()
• I don't know how long to allocate for a single listBatchSubscribe().
• If your site has n lists to update, then we need to allocate time for updating each of those lists.

I'm aiming to process as many users as possible within some sensible limits:

• Don't come within n seconds of max_execution_time.
• Don't try to update more than 5K users in a single batch.
• ... anything else you can think of?

Kind of feels like the batch processing approach is inevitable, even if we are only updating the emails we actually need to. Grrr.

(Random unscientific sample: 860-odd users all on a single list, 16 seconds on a 2.4GHz laptop)

Oh yeah. This bit *really* sucks IMO.

When we upgrade to the version which implements a queue, we can't assume that an empty queue is correct. In fact, the ONLY assumption we can fairly make (to keep peoples lists correct) is to assume that everyone's in the queue to begin with.

So ... yeah.

Looks like the update which adds this feature is going to have something like "add all current users to the queue". Batch processing is therefore required, because otherwise all the people currently stuck trying to update all users are going to be stuck trying to update all users out of a different queue. JOY.

Feedback welcome :)

Interesting points. I just think that we need the option of batch size to be configurable in the UI. Depending how much other stuff a site has running at cron (and how much memory all of this requires), the optimal size may be different for each site. Plus, it might take some guess and check to figure out what this optimal size may be. For our sites, we'd probably set a maximum limit much lower than 5,000.

--Ben

Hey gang - Just a quick note from the module maintainer to let you know I'm following this and whole heartedly support any improvements to the cron system. I'm a bit slammed now, and have a new baby on my hands, so can't contribute much code for the next few weeks, but if anyone wants to roll a patch, at least a starting point, I'll be happy to run with it and include in the module.

thx loubabe that really helps to know you're into it

try this for a patch

* adds a table mailchimp_pending_update (kinda think this is a dumb name tho)
* on install puts everyone in said table (should we exclude $account->status==0 from this?)
* adds a selector in admin/settings/mailchimp which sets how many users to process
* processes that many users (default 100) per cron run
* skips people where $account->status == 0

* we only queue updates & adds not deletes
* doesn't check the result of listBatchSub and put people back in the queue - only just thought of this!

congrats on the new baby loubabe!

(this comment brought to you with one handed punctuation due to my 7mo son mauling the netbook if he comes in range)

A small amendment to the previous one - calling hook_user() on the same user twice was attempting to add them to the queue, causing an INSERT DUPLICATE error. This one does DELETE ; INSERT.

Keen for feedback / testing pls!

Nice work, xurizaemon, patch looks great, although haven't tested in the wild. Do you all feel a separate db table is really necessary as opposed to just storing the pending UIDs in the variables table as a serialized array or some such approach? I've avoided adding db tables thus far ...

And thanks for the congrats!

I did look at that option (I had a previous workaround to hand which used that method) but the size of the serialized array made me balk at the approach.

This site had 54K+ users ... serialize() is not the most efficient storage mechanism to start with, packing that many integers with a bit of string padding and then calling serialize() and unserialize() as we iterated over the array really seemed wasteful.

I did take a careful look at about twenty other Drupal sites and compare the max size of values stored in the variables table on them to see. One had a huge long variable (it was an Acquia theme's settings FWIW) and the others seemed to cap at around 1.2Kb. Then I used this as a rough guide on what would be an acceptable variable size to use.

A quick test says for 55K uids, I'd be looking at an 850Kb value in the variables table.

php -r 'for ( $i=1;$i<55000;$i++ ) { $arr[]=$i ; } ; print(strlen(serialize($arr))) ;' = 857778 ...

Even a thousand users gets fatter than I'd like to see in my variables table:

php -r 'for ( $i=1;$i<1000;$i++ ) { $arr[]=$i ; } ; print(strlen(serialize($arr))) ;' = 11778

We might shave a bit off by not doing db_query() as we process each user, but we'd save that at the cost of a much fatter variable cache. That value would end up getting unserialized on each page load ... etc etc etc.

It's true that this would only affect most sites once (remembering we have to add everyone to the queue when migrating to the batch/queue method) but I still wouldn't be happy with that myself.

That's well reasoned, I'm convinced. Couple other suggestions:
* The record would need to be removed from mailchimp_pending_update when an account is deleted via the delete operation in hook_user().
* I think the query in hook cron should contain a limit clause and get passed the batch limit variable, rather than selecting all rows.
* Perhaps mailchimp_pending_update should also contain the newsletter ID, and the query should be called for each required list rather than the nested loop structure we have now.

Broader question, which I feel I should raise now since we're considering adding the db table. Currently, a users subscription status is determined by making a request to the MC API rather than local storage since a user could unsubscribe without the Drupal site knowing. Obviously this impacts loading the account pages. I was considering whether that feature was worth the overhead, or if a users subscription status should be stored locally. If the latter, than our new table would be of broader use, containing a uid, newsletter ID, status, and a flag if it needs to be updated. Thoughts?

Agreed about the LIMIT clause when selecting from the queue. Makes perfect sense, I guess I was still considering variable storage when I wrote that counter :)

I had wondered about caching more info in the local table - the use case I was considering was delete, because if we cached the email there we could also batch deletions as well.

In this patch I've added code to delete from the queue in hook_user('delete'), even though I don't think we actually need to delete from mailchimp_pending_update at this point:

1. The batch process will simply remove them from the queue after failing to load the user a/c.
2. _mailchimp_unsubscribe_user() is called in hook_user('delete')

--

This patch adds the following changes:

1. Delete from queue when user a/c is deleted.
2. Use SELECT ... LIMIT instead of counter.
3. Expanded docs and information in the settings screen (see attached image).

--

As a random sample, with a single required list and five fields merged from content profile, it takes about 50 - 60s to handle 1000 user updates.

--

I think your idea of local storage may be a good idea, but as people can unsubscribe via MC without notifying the Drupal site, it seems like this cache might end up being an unnecessary duplicate of data stored elsewhere (and then pollute the Mailchimp data, eg by re-subscribing someone who's removed themself offsite).

My 2c: store locally any data which relates only to the Drupal site (eg, "this is a list of UIDs which need updating to MC"), but don't duplicate data stored elsewhere ("this is a list of UIDs on the Foo list") because those details can change.

Storing them locally does make sense to me if we're pulling a local cache of the list subscribers to iterate over it, and throw away the list when we're done. (And if we do look at that, let's take it to a new issue; right now I just want cron to update our Mailchimp subscribers!)

Thanks for the patch, it looks like a good approach.

One minor nit pick though is that drupals preferred method of using the LIMIT sql clause is to use
db_query_range() due to differing LIMIT syntax between different database servers.

good point Agileware, thx!

xurizaemon, I just tested the patch and realized you're missing a hook_install() implementation for the new db table for users just installing the module.

Thanks LouBabe.

My (incorrect it seems) understanding was that this would be picked up by running mailchimp_update_6204() on install, but on reading jweowu's comment on hook_update_N() and [development] Doing hook_update_N() when module is installed it seems that's not the case.

Will check into this over the weekend on a fresh site, and add the hook_install() code as appropriate.

I won't mind if someone else adds it in, though :)

Here's an updated patch

It has the following changes:
* Added hook_install()
* In hook_uninstall() changed the DROP TABLE sql to use drupal_uninstall_schema()
* Added a few extra number options for the batch limit (can't hurt to give extra options)
* Changed hook_update_6204() to hook_update_6201()
* A few coding standards fixes. Mostly just fixing missing or excess spaces.

Thanks Agileware and xurizaemon, patch committed with a few minor tweaks.

Looks like we need to add this at line 76 to prevent INSERT DUPLICATE error when saving a user's account page a second time?

db_query('DELETE FROM {mailchimp_pending_update} WHERE uid = %d', $account->uid);

Yeah, that looks better.

Guys, I've tried a slightly different approach in the attached module. Basically, it creates a table called mailchimp_user with uid and status columns, and then does updates/inserts depending on the scenario. I also like this approach because we can use one schema and build on top of it for other use cases, such as adding list IDs and subscription status via Mailchimp's hook system. Anyways, please take a look and let me know your thoughts.

Looks good. I'd alter the info shown in the final line of your patch to be,

"There are currently !count users pending update in the queue."

and

db_result(db_query('SELECT COUNT(*) FROM {mailchimp_user} WHERE status = %d', MAILCHIMP_USERSTATUS_PENDING))

Good catch. The new approach is committed.

IMPORTANT: Since the previous schema was only committed for a couple days, I didn't bother including the changes as an update. So if you have mailchimp_pending_update installed, just delete it and re-run update 6201.

Hey everyone,

I just updated to the latest development snapshot, but unfortunately, my new users are no longer being sent to Mailchimp.

I've tried varying the batch limit setting, but none of the settings seem to be working. In my watchdog log, Cron is being run successfully, but it's running much too fast for the higher batch limits. Also, on my Mailchimp settings page, it says that "There are currently 10824 users with pending updates in the queue" but this number isn't changing.

I took a look at the mailchimp_user table and all users are listed with a "pending" status. Is this how it is supposed to be upon upgrading? I had upgraded from the Feb. 25, 2010 development snapshot.

Any thoughts or suggestions why this isn't working?

Thanks,
Ben

They should all be pending. While not ideal, that's how we decided to deal with upgrading to the new approach. At some point, they all need to updated and set to pending in order to track. As for why it's not working, not sure. There was a typo in the install hook. Maybe try reinstalling the newest version of the module.

I found the problem. It was an error in the usage of db_query_range, basically arguments being in the wrong order. Fixed, and now all works as expected.

I just tried the latest 6.x.-2.x-dev version and got the following error message:

"You have an error in your SQL syntax; check the manual that corresponds to your MySQL server version for the right syntax to use near 'LIMIT 0, 100' at line 1 query: LIMIT 0, 100 in /xxxxxxxxx/public_html/sites/all/modules/mailchimp/mailchimp.module on line 268."

It appears that in the latest committed code $sql is not being defined on Line 268 in mailchimp.module.

We fixed this by adding the following code on a new line above Line 268:

$sql = "SELECT uid FROM {mailchimp_user} WHERE status = '%s'";

Once this fix was added, batch cron processing seems to work properly.

Can we get this committed to the module as soon as you have a chance?

Thanks,
Ben

Fixed, and that sound, it's my hand slapping my forehead! Must have fat fingered a line delete before committing that last time, sorry. That's what you get committing patches late at night with too little sleep :)

Rather than re-open this issue, I have submitted another one that loubabe has pointed me it's realted to this one. I'd like if the main contributors of this issue's patches can check it out, and see my opinions there, maybe Agilware or grobot can submit a patch quicker than I as they've been working on the module for more time, but if not, at least to discuss there on that specific issue.

There might be more to come.

#839862: Auto-update of the userbase (remove update 6201?)

Is everyone using cron updates seeing people stay in Pending status?

#859326: Mailchimp cron does not move final user from queue.

Also related. Not actually sure there's a bug in this one, but worth reviewing for someone who (1) uses cron updates (2) has optional lists.

#859334: Update/sync optional lists as well as required lists with cron