Varnish: Drupal 7 version

This certainly needs a review =)

Let's start with coding standards. Everything seems fine at a first glance expect:

+++ varnish.module	5 Oct 2010 14:25:54 -0000
@@ -116,6 +139,8 @@ function varnish_comment($comment, $op) 
+ * ¶
+ * //@TODO update expire.module to drupal 7?
  */

Woops, a spacing here.

I will be coming back for a review on the functionality.

Powered by Dreditor.

I did a testing round with the module in Drupal 7, and it works.

However, I noticed and fixed the following:
* The varnish settings menu item should probably not be on the first level of settings =) I moved it to admin/config/development/varnish, where it shares the space with Performance settings.
* The status of varnish wasn't working. Changed #value to #markup, and removed the ul list, since it is quite unnecessary.
* Fixed numerous coding standards changes between 6 and 7.

The new attached patch contains the other patches listed in the previous entry. I added them since it will be much simpler to review, and the Drupal 7 version will have it's own branch anyway.

subscribing

The patch at #5 applies, apart from the .info chunk which is trivial. It seems to work (installs successfully, config appears, reports are generated) but I haven't got varnish configured yet so can't prove the important bit works.

+1 Subscribing

@willmoy: Did you fetch the latest from CVS? Everything applied fine when I tried.

Anyone else who can confirm that the info chunk fails?

Nope, I used the Nov 12 -dev tarball, so that explains that

This line looks obviously incorrect:
+ $items['admin/config/development/varnish'] = array(
Varnish has nothing whatsoever to do with development, and quite the contrary, is sth you would NOT use on a development site, but on a live site only.

This is the most logical and convention following location since Performance (caching, etc.) is also contained here.

subscribe

subscribe

subscribing

subscribe

What does the Varnish module do which D7 doesn't already provide? Certainly in pre-launch talk on the subject, it seemed like there would be as good native support for Varnish as there is for Pressflow. Am I missing something or were the claims exaggerated?

The varnish module has never provided support for Varnish and other reverse proxy cache servers. This is what we used Pressflow or patches for in D6.

What the Varnish module does is connecting to the varnish service port. It can then issue commands for purging parts of or all of the cache stored in Varnish. It is basicly a very handy tool to use when you have the need to purge your Varnish cache from Drupal.

The Varnish module could also potentially support other commands supported by the Varnish Service interface in the future.

Ok, that makes a lot of sense. Nicer to do that from drupal than have to jump to the command line.

subscribing...

Subscribing

Here is a reroll after the git migration.

Subscribing

Nevermind, I should get my facts straight before commenting.

Here is a patch on top of the varnish-927860-23.patch in comment #23 that uses Tim Whitlock's VarnishAdminSocket library. We upgraded a site in our AWS environment to D7 recently, and I had to scramble a bit to get the varnish module working in that environment quickly. We use the -S secret key option and the VarnishAdminSocket library worked really well with it.

I have a port here that we're using until the official D7 version comes out:
https://github.com/bshelton229/varnish

Reading the server list and secret key from files is easier for us the way our AWS environment is set up right now.

If anything in there looks interesting I'd be really happy to help.

The library looks like a good idea, and it's way more feature complete than what we have now, but I would be a little bit reluctant in adding at as a dependency, since installing libraries is a pain.

Also, I would like to know a little more about the library. The D6 version of this module is currently used by more than 1400 people, and I guess it works for most of them. We also have very competent people working on this module in the queue, so it might be a good idea to implement the functionality in the library ourselves, since that means we can have it in one place, which makes it easier to work on this project within the infrastructure of drupal.org

If we decide to roll with the library, we need to remove the library from the patch since it is MIT licensed, and therefore incompatible with drupal.org.

I will be working on the D7 version of the varnish module during the code sprint on friday at DrupalCon Chicago together with joshk. I hope everyone that's interested will join in!

Sorry about that, I completely spaced the licensing issue. Implementing any needed functionality directly definitely seems a better approach. I wish I could make the code sprint.

This needs a huge reroll after a lot of patches got commited. We are currently prioritizing getting the D6 version stable, and after that we will open a new branch, and incorporate some real changes in the D7 branch.

@fabsor, big thanks for cleaning up the queue. Rock on :)

Just re-titling for dashboard-goodness

Subscribing

sub

Subscribing!

suscribe

subscribe

subscribe

Can this work be checked in as a d7 dev branch? Or is there another initiative?

We are currently focusing on getting a stable D6 release out, and then base the D7 branch out of that. I will talk to Josh about how we can proceed further with the D7 release, and a road map for whats left for D6.

Also, for Drupal 7, this module needs to be rewritten as a cache implementation (replacing the page cache). This way it will be able to react to flush of the page cache directly, without needing any of the current hack in hook_nodeapi(), hook_comment(), hook_user(), hook_form_alter(), and the infamous hook_flush_caches().

@Damien, that's a very interesting idea. It would certainly make the module a lot cleaner :)

I run a patch on some of my D6 sites that has cache_clear_all() trigger a shutdown function which clears varnish. The results are iffy, since the whole cache is dumped extremely frequently. It works for sites that only have a few posts per day, but sites with a large number of commenters or logged in sessions will really suffer. Basically your cache will stick around for a total of 10 seconds at a time. On those types of sites, I don't use the varnish module and just rely on a shorter cache lifetime (15-30 minutes) instead.

@Damien That is a very interesting idea indeed. I will definitely have a look at that.

subscribe

subscribe

subscribing

also

To all subscribers: If you have time to review The issues outlined in #1127752: Roadmap: Drupal 6 stable release please do so! The faster we get those two patches in, the faster we can release a stable release of varnish for Drupal 6. A stable release for D6 means that the work on a version for D7 can begin. Thanks in advance and thank you for your interest in this project!

subscribe

subscribe

subscribe

Why do we need a stable D6 version to move forward on a D7 version?

Just a guess, from my own experience- because then you're not having to constantly port changes between branches.

FYI: #1169744: Varnish - 7.x-2.x battle plan

There is now a 7.x branch with a working port of the Varnish module. The only difference between that version and the D6 version is that you need to specify the provided cache plugin as the cache controller for cache_page, see the front page for details. Please test the new version and report any bugs you find as separate issues.

Thanks for all the interest in this module! I hope the D7 version will work well for you, so we can release a first stable release as fast as possible! =)