Problem/Motivation

On high traffic sites, we need more control over when nodes get flushed when comments are updated or recommended (voted). It may be detrimental to flush a node from Varnish whenever a comment is posted, especially if comment activity for any particular node is very high. We also don't want to flush the node if the comments appear on a separate page. Same goes for the votingapi.

Proposed resolution

Allow other developers to write their own flushing logic for comments and for the votingapi. This means disabling those hooks. In expire_admin_settings_form(), I'm recommending:

  $form['extra'] = array(
    '#type' => 'fieldset',
    '#title' => t('Optional expiration logic'),
  );
  $form['extra']['expire_flush_comments'] = array(
    '#type' => 'checkbox',
    '#title' => t('Expire node when comment is modified'),
    '#default_value' => variable_get('expire_flush_comments', EXPIRE_FLUSH_COMMENTS),
    '#description' => t('When a comment is modified, flush the node attached to the comment.'),
  );
  $form['extra']['expire_flush_voting'] = array(
    '#type' => 'checkbox',
    '#title' => t('Attach expiration logic to voting api'),
    '#default_value' => variable_get('expire_flush_voting', EXPIRE_FLUSH_VOTING),
    '#description' => t('Flush nodes when voting api events occur.'),
  );

In _expire_votingapi():

  // Return if admin wants to disable this module's votingapi hooks.
  if (variable_get('expire_flush_voting', EXPIRE_FLUSH_VOTING) === FALSE) {
    return;
  }

In expire_comment():

  // Return if admin wants to disable this module's hook_comment().
  if (variable_get('expire_flush_comments', EXPIRE_FLUSH_COMMENTS) === FALSE) {
    return;
  }

And at the top of expire.module:

define('EXPIRE_FLUSH_COMMENTS', TRUE);
define('EXPIRE_FLUSH_VOTING', TRUE);

Let me know what you think. We can't use this module unless we can write our own hook_comment to include custom flushing logic (for example, only flush Varnish if the comments appear on the same page as a node and it's been more than 5 minutes since the last comment).

CommentFileSizeAuthor
#8 expire-expiration_logic_optional-d7-1299358-8.patch15.69 KBjaydub
#6 1299358-expiration_configuration-d7.patch16.55 KBjaydub
#6 1299358-expire-optional-expiration-d6.patch11.52 KBjaydub
#3 1299358-expire-optional-expiration.patch7.12 KBmsonnabaum

Comments

jaydub’s picture

jaydub commented

I wrote a patch to configure node types to exclude from expiration here #1363218: Allow node types to be excluded from automatic expiration. It is only processed inside the hook_nodeapi call so it would not be effective for the forcing comments and/or votingapi expirations actions from occurring but if I moved the exclude node type check to the expire_node() function then it would be used by the comments and votingapi expirations. Would this be enough to support what you are asking for?

djbobbydrake’s picture

djbobbydrake commented

jaydub, we would still need the ability to prevent flushing on comments and votingapi actions. Our site allows anonymous users to recommend articles and comments. If we are purging cache whenever someone hits the recommend button, that can potentially take our site down.

msonnabaum’s picture

msonnabaum commented
Title: Give developers more control over comment and votingapi purges » Make all expiration logic optional
Version: 6.x-1.0 » 7.x-1.x-dev
StatusFileSize
new 1299358-expire-optional-expiration.patch7.12 KB

While the current expiration logic is thorough, it is not going to work for all sites. It would be best if all of it was optional so that you can implement only what you need or specify your own logic with rules.

The attached patch is against the D7 branch which incorporates the changes described above as well as new options for node and user expiration. It has the added benefit of making optional the parts of this module that are currently very broken in D7 so that this branch can at least be used with rules.

djbobbydrake’s picture

djbobbydrake commented

+1 on the patch in #3

jaydub’s picture

jaydub commented

I'm working on a patch for d6 for this and noticed that the logic for the test against the variables for whether or not to expire comments/nodes/users is backwards:

@@ -44,7 +48,7 @@ function expire_menu() {
  */
 function expire_comment_insert($comment) {
   // Return if no node id is attached to the comment.
-  if (empty($comment->nid)) {
+  if (empty($comment->nid) || variable_get('expire_flush_comments', EXPIRE_FLUSH_COMMENTS)) {
     return;
   }

The variable_get() call here in the hook_comment_*, hook_nodeapi_* and hook_user_* implementations should be of the form:

+ if (empty($comment->nid) || !variable_get('expire_flush_comments', EXPIRE_FLUSH_COMMENTS)) {

since this is the test for whether or not to return early in the function w/o processing any expirations. Obviously if variable_get() is TRUE as it would be if you wanted to expire comments then the current patch would actually never process expirations.

jaydub’s picture

jaydub commented
StatusFileSize
new 1299358-expire-optional-expiration-d6.patch11.52 KB
new 1299358-expiration_configuration-d7.patch16.55 KB

Ok I've got a big patch for d6 and d7 that incorporates the request for an option to enable/disable expiration of nodes, comments, users or votingAPI events. I've folded in the patch from #1363218: Allow node types to be excluded from automatic expiration as well so that you can also exclude certain node types from expiration which in the end should allow for the best of all worlds configurability.

The UI gets a little busy in this case so I threw in a quickie JS to show/hide the options for nodes, comments, etc depending on whether the main option to enable/disable expiration of nodes, comments, etc is checked.

jaydub’s picture

jaydub commented
Status: Active » Needs review
jaydub’s picture

jaydub commented
StatusFileSize
new expire-expiration_logic_optional-d7-1299358-8.patch15.69 KB

Updated patch for drupal 7 branch now that I'm back to testing Expire/Purge with Varnish.

spleshka’s picture

spleshka
he/him
Primary language English
Location UAE
commented
Status: Needs review » Fixed

Please, check out new 7.x-2.x branch. There are a lot of options for cache expiration. Moreover, you may set your own expiration rules using Rules + Cache expiration modules together.

spleshka’s picture

spleshka
he/him
Primary language English
Location UAE
commented
Version: 7.x-1.x-dev » 7.x-2.x-dev

Forgot to change version number.

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.

Anonymous’s picture

(not verified) commented
Issue summary: View changes

fixing formatting in code suggestions