Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support cache tagging #2

Merged
merged 1 commit into from
Dec 30, 2013
Merged

Support cache tagging #2

merged 1 commit into from
Dec 30, 2013

Conversation

ddeboer
Copy link
Member

@ddeboer ddeboer commented Dec 27, 2013

Tag your HTTP cache entries, and later invalidate cache entries by tags. This is sometimes called Tagged Cache Invalidation, a simpler version of Linked Cache Invalidation. See also this Drupal proposal.

For Varnish, this will come down to setting a custom header on the response and then banning on that header later.

By the way, Varnish have their own implementation called hashtwo that's probably more efficient, but as it's a paid solution, it's less relevant to us.

See also http://www.gossamer-threads.com/lists/varnish/misc/19732#19732

@dbu
Copy link
Contributor

dbu commented Dec 14, 2013

afaik SonataCacheBundle also does something like this, among many other things. we might inspire ourselves there to see if we can generalize that.

@ddeboer
Copy link
Member Author

ddeboer commented Dec 14, 2013

Also found this.

@ddeboer
Copy link
Member Author

ddeboer commented Dec 14, 2013

Perhaps the most elegant solution is to have one Cache\Tags annotation by which you could add cache tags to controller actions.

  • When a tagged action processes a safe request (GET), the tag is added to its response as a custom header.
  • When a tagged action processes a non-safe request (PUT, POST, DELETE, PATCH), an invalidation (BAN) request for the tag would be sent out.

@dbu
Copy link
Contributor

dbu commented Dec 14, 2013

wouldn't the parameters to the cache response and for invalidation be quite different? and for GET its the sensio framework extra bundle that provides the tag, i don't think we can just extend that tag.

@ddeboer
Copy link
Member Author

ddeboer commented Dec 27, 2013

This PR is stil incomplete, as it calls BanInterface::ban() with a headers param, which doesn't yet exist on the interface. Should we add a fourth param to the ban method, or should we start separating the methods along the lines of:

  • banUrl($url, $contentType, $hosts)
  • banHeaders(array $headers)

@dbu
Copy link
Contributor

dbu commented Dec 27, 2013

well, ban is all about sending requests with any headers and a special type BAN to varnish. indead, we could have ban(array $headers) allowing to do anything (including a header for the url if needed) and a convenience banUrl with url, content type and hosts array for those that do not want to build the headers themselves.

@ddeboer
Copy link
Member Author

ddeboer commented Dec 28, 2013

we could have ban(array $headers)

Good point. I made ban method simpler and created a convenience banPath method. This PR is ready for review and merge.

@@ -34,13 +38,38 @@ class CacheManager
* @param HttpCacheInterface $cache HTTP cache
* @param RouterInterface $router Symfony router
*/
public function __construct(HttpCacheInterface $cache, RouterInterface $router)
public function __construct($cache, RouterInterface $router)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why did you remove the interface?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because the HTTP proxy classes (Varnish, Nginx, ...) implement (at least one of) BanInterface, PurgeInterface and RefreshInterface, there's nothing left in the HttpCacheInterface. Would you prefer to have the proxies implement an empty HttpCacheInterface instead?

@dbu
Copy link
Contributor

dbu commented Dec 29, 2013

very cool! i commented on a few details. feel free to merge (and maybe squash the commits).

btw, i will be in the mountains from tomorrow on for a week, and have no computer with me.

@ddeboer
Copy link
Member Author

ddeboer commented Dec 29, 2013

Thanks for your comments.

Enjoy your trip! Being away from computers for some time can be a very good thing. 😉

Fix class name

Fix typo

Clean up test

Improve code and add docs

Add another expression example

Add check for characters in tag name that clash with regex

Refactor BanInterface

Rename ban to banPath and implement simpler ban method

Make cache tags HTTP header settable

Add note on dependencies

Re-add cache proxy interface

Make Varnish tests compatible with refactored BanInterface

Make functional test case easier to use

Make separate case for when tags are banned

Fix banning tags and add functional test

Don't flush Varnish if no requests are queued

Add flush expectation

Explain how to configure Varnish for cache tagging

Fix link in docs
ddeboer added a commit that referenced this pull request Dec 30, 2013
@ddeboer ddeboer merged commit 6d4c770 into master Dec 30, 2013
@ddeboer ddeboer deleted the tagging branch December 30, 2013 20:26
*/
public function __construct(HttpCacheInterface $cache, RouterInterface $router)
public function __construct(CacheProxyInterface $cache, RouterInterface $router)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ah, now i get it. should BanInterface etc all extend CacheProxyInterface then? it means they are "a kind of cache proxy" which seems to be what we are saying.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants