Remove throttle module again

Okay, here's one properly showing in the diff that the file is removed. (Note to self: Use diff -N after having removed the file.)

Looks good otherwise. This one also fixes a few trailing whitespaces still remaining. Going to test and look more properly through now, but thought I'd throw this patch up for now, for anyone else wanting to give this a look. ;)

freso@nayru /s/h/l/h/drupal7> patch -p0 < ../patches/throttle.patch
patching file CHANGELOG.txt
patching file includes/install.inc
patching file includes/module.inc
patching file includes/session.inc
patching file modules/block/block-admin-display-form.tpl.php
patching file modules/block/block.admin.inc
patching file modules/block/block.install
patching file modules/block/block.module
patching file modules/statistics/statistics.module
patching file modules/system/system.admin.inc
patching file modules/system/system.install
patching file modules/system/system.module
patching file modules/throttle/throttle.admin.inc
patching file modules/throttle/throttle.info
patching file modules/throttle/throttle.module
freso@nayru /s/h/l/h/drupal7> grep -ir "throttl" *
CHANGELOG.txt:- Removed throttle module:
CHANGELOG.txt:    * The throttle module has been removed. Alternative methods to improve
CHANGELOG.txt:    * Impose a throttle on indexing of large sites.
CHANGELOG.txt:    * Added a mechanism to throttle certain operations.
CHANGELOG.txt:    * Refactored the throttle module configuration.
CHANGELOG.txt:- Added throttle.module:
CHANGELOG.txt:    * Auto-throttle congestion control mechanism: Drupal can adapt itself based on the server load.
CHANGELOG.txt:- Rewrote watchdog and submission throttle:
modules/CVS/Entries:D/throttle////
modules/search/search.admin.inc:  // Indexing throttle:
modules/search/search.admin.inc:  $form['indexing_throttle'] = array('#type' => 'fieldset', '#title' => t('Indexing throttle'));
modules/search/search.admin.inc:  $form['indexing_throttle']['search_cron_limit'] = array('#type' => 'select', '#title' => t('Number of items to index per cron run'), '#default_value' => variable_get('search_cron_limit', 100), '#options' => $items, '#description' => t('The maximum number of items indexed in each pass of a <a href="@cron">cron maintenance task</a>. If necessary, reduce the number of items to prevent timeouts and memory errors while indexing.', array('@cron' => url('admin/reports/status'))));
modules/system/system.install: * Remove throttle columns and variables.
modules/system/system.install:  db_drop_field($ret, 'blocks', 'throttle');
modules/system/system.install:  db_drop_field($ret, 'system', 'throttle');
modules/system/system.install:  variable_del('throttle_user');
modules/system/system.install:  variable_del('throttle_anonymous');
modules/system/system.install:  variable_del('throttle_level');
modules/system/system.install:  variable_del('throttle_probability_limiter');
modules/throttle/CVS/Repository:drupal/modules/throttle
modules/throttle/CVS/Entries:/throttle.admin.inc/-1.2/Tue Jan  8 10:35:43 2008//
modules/throttle/CVS/Entries:/throttle.info/-1.5/Wed Feb 20 19:37:22 2008//
modules/throttle/CVS/Entries:/throttle.module/-1.83/Fri Dec 14 18:08:49 2007//
sites/all/modules/coder/tests/coder_style.inc:<p>If you want certain blocks to disable themselves temporarily during high server loads, check the "Throttle" box. You can configure the auto-throttle on the <a href="@throttle">throttle configuration page</a> after having enabled the throttle module.</p>
sites/all/modules/coder/tests/coder_style.inc:<p>You can configure the behaviour of each block (for example, specifying on which pages and for what users it will appear) by clicking the "configure" link for each block.</p>', array('@throttle' => url('admin/settings/throttle')));
sites/all/modules/coder/coder.module:      'throttle' => 1,

As can be see from the above grep, the only remains of throttle are either expected on in contrib land. Site seems to operate fine without throttle as well, and the update runs smoothly. :)

I wasn't in Boston... but why don't we need this module anymore to reduce server load? Should i see block caching as a replacement or is there anything other new I'm not aware about?

This looks pretty good... although the actual removal of the throttle module seems to have gotten lost between #3 and #5 ;) Getting rid of all that entangled code is definitely very nice.

However, am I right in thinking that if this patch is applied, it won't be possible to have a throttle module at ALL (even in contrib)? If so, that might not be so good. I see what you're saying that in most cases aggressive caching is a better solution, but I don't think that's always true. (For example, you might be running other modules on your site which separately prevent you from using aggressive caching. Or you might have a very expensive module enabled that you want to throttle even for logged-in users.)

Just looking at your patch, it seems like there are only two real places in core where the throttling itself actually takes place (not counting that random thing in the statistics module, which seems unnecessary). These are module_list() and _block_render_blocks(). In both cases, the pattern is basically "generate a list of 'things' (modules or blocks), and skip over the throttled ones."

I am just wondering to what extent a new hook could be added that would let contrib modules do stuff here? Something like hook_module_list_alter() or hook_block_list_alter (or maybe combine them into a single more generic hook). Basically, something where Drupal says "I just made a list of things I'm about to do some processing on; does anyone want to remove things from that list before I do it?"

Hm, I guess hook_module_list_alter() might be tricky (given where it's called from it can't really be a traditional "hook"). But perhaps there's still a way. If so, this would let Throttle easily become a contributed module, and maybe such hooks could be useful in other scenarios too? They could definitely allow for a richer variety of throttling solutions in contrib; for example, a "throttle by role" option that throttles the site for some users but allows others (e.g., site admins) to still get the full functionality.

Anyway, just thinking out loud here ;)

Patch no longer applies, but otherwise looks good.

Heh. Catch had gotten the patch for #244942: outdated function name in schema description mixed into this, and since that has already been checked in, that hunk obviously failed. Re-rolled patch without this hunk. :)

This is ready... :-)

You forgot the "throttle" column in the "system" table (it was included in your earlier patches). Otherwise looks good, so I'll keep the RTBC. :)

(Oh, and I also found and removed a trailing white-space with the grep regex I showed in IRC yesterday. ;))

A visual review says that all previously reported problems are fixed in this one (and my grep regex doesn't catch any trailing white-space ;)). Waiting for a second pair of eyes and hands to look it over before marking RTBC, but I think it's there.

@catch: OK, cool, thanks. You're right that block throttling has very limited use cases now (and besides, that hook would be easy to add if it's ever needed). And it sounds like something along the lines of hook_module_list_alter() will be possible too, although the best way to do it is still up in the air. All sounds good. I just wanted to check that there is some kind of foreseeable path whereby this might be able to eventually exist in contrib... seeing as how there is, I guess it's time to say R.I.P. to the throttle module ;)

I tested the patch in #16 both with a new installation and an upgrade from D6 (with throttle module enabled). I also reviewed the code and did a final "grep" to make sure the throttle stuff is all gone. Everything seems to work fine...

And http://drupal.org/cvs?commit=111061 too ;) But throttle.admin.inc is still in the repos though...

Finally removed them all now ...

There, there Dries. It's gone now. :)

( http://drupal.org/cvs?commit=111284 )

Finally removed them all now ...

wwe

This is like a bad horror movie... the throttle module just won't die! I noticed that somehow, the throttle column is still hanging around the {system} table in the database! Not sure what happened since the patch that was committed sure looked like it got rid of it. Believe it or not, I think that somehow there were two copies of the 'throttle' definition in system_schema(), and the committed patch only got one of them and left the other behind.

Anyway, this should kill it for good :)

Committed to CVS HEAD. For real, now. Thanks David.