[WIP] Feature EZP-25696: HTTPCache Multi Tagging/labeling by andrerom · Pull Request #1772 · ezsystems/ezpublish-kernel

andrerom · 2016-09-08T15:25:08Z

Epic: https://jira.ez.no/browse/EZP-25696
Meta PR: ezsystems/ezplatform#143

As we have own layers for HttpCache it is possible to solve this without waiting for others.

TL;DR; Multi tagging for for all solving many edge cases, very simple multi tagging support added for Symfony HttpCache Proxy in our own cache store layer, meant to be replaced by using Symfony Cache component once we move to Symfony 3.x, and change to xkey header to align with change on Varnish side to use xkey vmod for better (more stable) performance.

Solves:

Performance:

Makes it possible to cache anything, example included is ContentType REST calls
Less work on purge, no need to iterate the content tree to calculate what needs to be cleared
Better Varnish performance by move to xkey over Banning to allow soft purging (grace handling)
With this we can set default cache TTL much higher, as it more reliably clears relevant cache now
Makes it possible to combine several responses more reliably in one response (needed for REST Embedding to speed up Platform UI by reducing round trips on thins like content > main location and more importantly for Permissions lookup REST API to not greatly slow down UI further)

Issues:

Solves lots of cache clearing issues, edge cases, especially when deleting content (hence possible to set higher TTL as written above)

BC

This change on purpose rather intrusive to HTTPCache layer, not all strictly needed to solve the issues, but overdue as it was somewhat jungle in there and cleanup highly needed as it has received changes over the years.

see doc/bc/changes-6.7.md
not mentioned yet, but as smart cache clearing won't come back in the form it current has the PR also deprecates and strips out use of GatewayCachePurger and implementations in favor of just the PurgeClientInterface layer.

Testing

See meta PR.

Todo

VCL, change to only have one: for Varnish 4.x (and 5.x as it has same syntax) with xkey installed
Fixes to FOSPurgeClient once VCL changes are in place to align
Fix inline todos
Remove the added RelationView stuff and instead document and adjust for setting such logic inline in templates when relation fields are viewed so changes on the relation goes back to self (the reverse relation) when need only

Future

(2.x): Refactor TagAwareStore to just simply use Symfony Cache component once 3.2 is out to have more solid tag handling, and built in expiry support natively to reintroduce staleness (however we need to look into how to avoid staleness for Platform UI requests as it will easily break the UI as mentioned in regards to Varnish above)

Todo: - [x] Add cache for all load operations used by UI and other clients -- [x] Add slots to trigger tag clearing on more operations (content type, sections..) - [ ] Adjust existing CachedValue use to add more relevant tags and not just locationId

[Skip CI]

bdunogier

Partial review, but some elements can be discussed already.

bdunogier · 2016-09-14T20:50:26Z

+  - `LOCATION_CACHE_DIR` and `LOCATION_STALE_CACHE_DIR` and related functionality is gone as tags are used instead
+  - See `doc/specifications/cache/multi_tagging.md` for further details.
+
+* Smart cache clearing is currently disabled, and might come back in form where it generate tags to clear,


Could we list what will be affected by that ? Mostly thinking about clearing that used to be performed, and wouldn't anymore with these changes.

bdunogier · 2016-09-15T09:24:12Z

+opposed to that it is done.
+
+But to avoid stale cache issues caused by cache being refreshed before search index is or similar, it is now important
+that HTTP Cache ttl *(default_ttl)* is not set to high, especially for REST endpoints used by UI.


Typo: s/to/too/.

bdunogier · 2016-09-15T10:05:52Z


-        $this->cacheManager->invalidate(array('X-Location-Id' => '^(' . implode('|', $locationIds) . ')$'));
+        // @todo: Need to take advantage of xkey if enabled to avoid BAN unless VCL handles this for us.
+        $this->cacheManager->invalidate(array('xkey' => '^(' . implode('|', $tags) . ')$'));


Short array syntax while at it ?

bdunogier · 2016-09-15T10:06:00Z

    public function purgeAll()
    {
-        $this->cacheManager->invalidate(array('X-Location-Id' => '.*'));
+        $this->cacheManager->invalidate(array('xkey' => '.*'));


Short array syntax ?

bdunogier · 2016-09-26T09:41:03Z

+
+- `content-type-<content-type-id>`
+
+*Tagging*: Used for tagging content responses with type id.


What about other response types, such as content type ? We don't currently cache responses from the content type controller, but if we have content-type tags, we can very do it.

Note that the PR's description gives ContentTypes as a first example of what could be cached thanks to this.

In that case, the HttpCache slots on ContentType service are missing for this.

bdunogier · 2016-09-26T09:41:20Z

+*Clearing*: When a operation is specifically affecting parent location(s), on tree operations `path` is more relevant.
+
+- `path-<path-location-id>`
+


bdunogier · 2016-09-26T13:10:36Z

+*Clearing*: When a operation is specifically affecting just given content.
+
+
+- `content-type-<content-type-id>`


What about content-type-group-<content-type-group-id> ? It would be useful for list of content types by group, to expire those when a new content-type is added to the group.

bdunogier · 2016-09-27T09:17:03Z

+and the signals will need to be expanded to contain the relevant info depending on operation they correspond to.
+
+
+### Predefined System Tags


Do we want to add a tag for Roles as well ?

Conflicts: doc/bc/changes-6.5.md eZ/Publish/Core/REST/Server/Controller/Content.php

This reverts commit c1f4a68.

This reverts commit 7cd936e.

…llowup PR With exception of header name and value change in CachedValue to make sure current tests passes.

Conflicts: eZ/Publish/Core/MVC/Symfony/Cache/Http/LocationAwareStore.php eZ/Publish/Core/MVC/Symfony/Cache/Http/SignalSlot/PurgeForContentHttpCacheSlot.php eZ/Publish/Core/MVC/Symfony/Cache/Tests/Http/LocationAwareStoreTest.php

andrerom · 2016-11-12T16:01:51Z

+    /**
+     * Returns the Relations for the current entity being viewed.
+     *
+     * @return \eZ\Publish\API\Repository\Values\Content\Relation[]


@bdunogier Note: I assumed you would prefer full object, but for tag needs I just need to keep id's around, and if we did that we could optimize the loading of relational data to only be one spi calls instead of 1+n (where n is number of relations). The design of this does not make a differance when cached, but it does make a differance in dev (either on cold cache or with cache disabled).

Input from @bdunogier, instead of doing this no matter if relation is rendered or not we should use manual tagging in this case. there is a twig function by fos_http_cache for this.

For some reason host is not being applied, so this might be a workaround for something else going on here. The exception given is: https://github.com/FriendsOfSymfony/FOSHttpCache/blob/1.4/src/ProxyClient/Varnish.php#L141-L145

bdunogier

Managed to dive a bit deeper into it.

Overall, I think it's good. I still have doubts in regards to tagging responses. As discussed face-to-face, we might be able to make that MUCH easier for developers by tracking API calls, and tagging the response based on API values that have been loaded.

But maybe it's too much for the time being ?

Do you want me to take care of ~~the~~ some of the todos, or did you intend to do it yourself ?

bdunogier · 2016-11-14T15:24:51Z

+
+#### Other eZ Repository domains
+
+Tags for other domains in the Repository also have tags, e.g.:


There can be only one "tag" in this sentence, methinks :)

bdunogier · 2016-11-15T10:34:53Z

@@ -1,255 +0,0 @@
-<?php


BC: could we leave the file where it is, and mark it as deprecated ? It doesn't cost anything as long as we handle removals for 7.0.

but if someone use it they won't get a clear error saying they need to adapt, they'll continue using that and it won't work as headers are different.

Hmm, we can add deprecation warnings, can't we ?

They will for sure notice if they were using it and we remove it :)

Hmm, we can add deprecation warnings, can't we ?

we can, so you prefer that then, and that deprecation warning says class most likely won't work anymore.

Well, the warning should say which class replaces it. The class works, it's just not used anymore. If somebody has a service that uses it, it will throw the warning during container compilation.

Unless you're really against it, I prefer it that way. Thank you :)

bdunogier · 2016-11-15T10:37:57Z

+
+    /**
+     * Injects a Filesystem instance
+     * For unit tests only.


I'd maybe prefer if we removed this from the class, and handled this in unit tests:

either using reflection

or by making $fs protected and adding a test class that extends TagAwareStore and adds this method.

Just saw that the getCachePath method below is public for the same reason, but it could be solved if it was a dep, as suggested.

bdunogier · 2016-11-15T10:38:26Z

+            $this->fs = new Filesystem();
+        }
+
+        return $this->fs;


Why do we need this ?

As in why do we need getFilesystem()? I'll have to check, as many of your other questions it's inherited from the previous class.

The question was actually why we needed the if (!isset())... new part.

The fact that it comes from the parent class is a good reason.

bdunogier · 2016-11-15T10:39:15Z

+        $digest = $response->headers->get('X-Content-Digest');
+        $tags = $response->headers->get('xkey', null, false);
+
+        if ($response->headers->has('X-Location-Id')) {


Should we also remove the header ? Or is this handled somewhere else ?

This can be logged as separate enhancement, it was not covered before, and parent Store class does not seem to strip anything either.

bdunogier · 2016-11-15T10:59:14Z

+    public function purgeAllContent()
+    {
+        $cacheTagsCacheDir = $this->getTagPath();
+        $this->getFilesystem()->remove($cacheTagsCacheDir);


This method is supposed to return bool. Since remove doesn't return anything, maybe we should catch exceptions it sends, and return true/false depending if something was caught ?

I'll need to check against old class, might be we can adjust doc here.

bdunogier · 2016-11-15T11:00:01Z

+                }
+            }
+            $fs->remove($files);
+            // we let folder stay in case another process have just written new cache tags


Missing return true.

bdunogier · 2016-11-15T11:03:36Z

+            // Flip the tag so we put id first so it gets sliced into folders.
+            // (otherwise we would easily reach inode limits on file system)
+            $tag = strrev($tag);
+            $path .= DIRECTORY_SEPARATOR . substr($tag, 0, 2) . DIRECTORY_SEPARATOR . substr($tag, 2, 2) . DIRECTORY_SEPARATOR . substr($tag, 4);


Could be:

$path .= implode( DIRECTORY_SEPARATOR, array_slice(str_split(strrev($tag), 2), 0, 4) );

Not sure it's better, but it's different. Maybe easier to make configurable (folder length, number of folders).

Did you consider making this an external service ? It's an important responsibility, and it could be worth testing individually.

I kept it like this as this is how it is done upstream in Symfony for getPath(), I also find this more readable then the example you propose even if it indeed avoids the long line.

Okay, no problem.

andrerom · 2016-11-22T00:45:23Z

+ * @copyright Copyright (C) eZ Systems AS. All rights reserved.
+ * @license For full copyright and license information view LICENSE file distributed with this source code.
+ *
+ * @version //autogentag//


andrerom · 2016-11-22T00:46:33Z

+            $this->fs = new Filesystem();
+        }
+
+        return $this->fs;


As in why do we need getFilesystem()? I'll have to check, as many of your other questions it's inherited from the previous class.

andrerom · 2016-11-22T00:48:05Z

+    public function purgeAllContent()
+    {
+        $cacheTagsCacheDir = $this->getTagPath();
+        $this->getFilesystem()->remove($cacheTagsCacheDir);


I'll need to check against old class, might be we can adjust doc here.

andrerom · 2016-11-22T00:48:32Z

+ * @copyright Copyright (C) eZ Systems AS. All rights reserved.
+ * @license For full copyright and license information view LICENSE file distributed with this source code.
+ *
+ * @version //autogentag//


andrerom · 2016-11-22T00:48:47Z

+ * @copyright Copyright (C) eZ Systems AS. All rights reserved.
+ * @license For full copyright and license information view LICENSE file distributed with this source code.
+ *
+ * @version //autogentag//


andrerom · 2016-11-23T17:05:48Z

-  Examples:
-   - (123|456|789) => Purge locations #123, #456, #789.
-   - .* => Purge all locations.
+- If "http" is used, only one Http PURGE request will be sent, with xkey header containing tags to purge seperated by space.


fix, several request are done per key, as per what xkey supports.

andrerom · 2016-11-24T11:54:32Z

Do you want me to take care of the some of the todos, or did you intend to do it yourself ?

@bdunogier Missed that comment in the reviews, I can take care about it today, but let's sync on it so we find a good fit for doc. BTW any tought on #1772 (comment)

[skip ci]

bdunogier · 2017-02-08T18:14:09Z

Replaced by http://github.com/ezsystems/ezplatform-http-cache. Please do not delete the branch yet.

André R added 9 commits September 7, 2016 12:04

Cleanup: Remove deprecated X-Group-Location-Id

111c2e2

TMP Multi tagging

1d925d4

Add some more todos and inline doc for readability

3304241

Publish: Make sure to clear all relevant cache on publish

3941f6e

Change TagAwareStore to be able to handle many tags

3426818

[Tests] Refactor tests to use OOP and not static galore

1ad329f

CS

62d8243

[Skip CI]

Test: Fix path seperator to pass on Windows

371bd05

bdunogier reviewed Sep 26, 2016

View reviewed changes

bdunogier reviewed Sep 27, 2016

View reviewed changes

bdunogier mentioned this pull request Sep 28, 2016

EZP-26297: REST embedding http cache integration #1789

Closed

8 tasks

TMP doc fixes

d6b5d6f

andrerom mentioned this pull request Oct 17, 2016

Symfony HTTP Cache / Tag Invalidation FriendsOfSymfony/FOSHttpCache#286

Closed

André R added 5 commits November 4, 2016 14:59

Merge branch 'master' into feature-EZP-25696

3423547

Conflicts: doc/bc/changes-6.5.md eZ/Publish/Core/REST/Server/Controller/Content.php

Revert "Disabled HTTP caching on REST content draft load"

7cd936e

This reverts commit c1f4a68.

Fix review notes

ab4954f

Revert "Revert "Disabled HTTP caching on REST content draft load""

bfff7fa

This reverts commit 7cd936e.

Remove changes to REST as it will be handled in a differant way in fo…

1f14100

…llowup PR With exception of header name and value change in CachedValue to make sure current tests passes.

andrerom force-pushed the feature-EZP-25696 branch from fb76ba6 to 1f14100 Compare November 4, 2016 15:19

André R added 4 commits November 11, 2016 18:23

Merge branch 'master' into feature-EZP-25696

5137492

Conflicts: eZ/Publish/Core/MVC/Symfony/Cache/Http/LocationAwareStore.php eZ/Publish/Core/MVC/Symfony/Cache/Http/SignalSlot/PurgeForContentHttpCacheSlot.php eZ/Publish/Core/MVC/Symfony/Cache/Tests/Http/LocationAwareStoreTest.php

Change to use purge for purging tags as supported by xkey

4e90744

Update deprecation version numbers

6f4627b

phpdoc cs

1d7ec4c

andrerom mentioned this pull request Nov 12, 2016

[WIP][Docker] Add support for using Varnish with xkey ezsystems/ezplatform#143

Closed

2 tasks

André R added 2 commits November 12, 2016 15:17

Adjust unit tests

a5dfb01

Implement RelationView, to be able to tag response with relations

4ec7994

andrerom force-pushed the feature-EZP-25696 branch from af04a9f to 4ec7994 Compare November 12, 2016 15:36

andrerom commented Nov 12, 2016

View reviewed changes

Fix missing Host as needed by Varnish ProxyClient

e2f6d56

For some reason host is not being applied, so this might be a workaround for something else going on here. The exception given is: https://github.com/FriendsOfSymfony/FOSHttpCache/blob/1.4/src/ProxyClient/Varnish.php#L141-L145

bdunogier reviewed Nov 15, 2016

View reviewed changes

andrerom commented Nov 22, 2016

View reviewed changes

andrerom force-pushed the feature-EZP-25696 branch from 042f22e to 7ceebce Compare November 23, 2016 17:00

Fix some of the review notes

136a7c1

andrerom force-pushed the feature-EZP-25696 branch from 7ceebce to 136a7c1 Compare November 23, 2016 17:04

andrerom commented Nov 23, 2016

View reviewed changes

TMP Start on cleanup for TagAwareStore

5c84914

[skip ci]

andrerom force-pushed the feature-EZP-25696 branch from 3bcc152 to 5c84914 Compare November 25, 2016 11:15

André R added 4 commits December 17, 2016 18:20

Finishing fixes on TagAwareStore

3d75a56

Merge branch 'master' into feature-EZP-25696

b810c88

Merge branch 'master' into feature-EZP-25696

69adb12

Update version numbers to 6.8

599789c

bdunogier mentioned this pull request Jan 26, 2017

WIP: Http multi-tagging signal slots #1897

Closed

2 tasks

bdunogier closed this Feb 8, 2017

andrerom deleted the feature-EZP-25696 branch February 8, 2017 18:15

andrerom restored the feature-EZP-25696 branch February 8, 2017 18:15

andrerom deleted the feature-EZP-25696 branch February 8, 2017 18:15

andrerom restored the feature-EZP-25696 branch February 8, 2017 18:15

andrerom deleted the feature-EZP-25696 branch April 20, 2017 19:45


		- `content-type-<content-type-id>`

		Tagging: Used for tagging content responses with type id.

		Clearing: When a operation is specifically affecting parent location(s), on tree operations `path` is more relevant.

		- `path-<path-location-id>`

		and the signals will need to be expanded to contain the relevant info depending on operation they correspond to.


		### Predefined System Tags


		#### Other eZ Repository domains

		Tags for other domains in the Repository also have tags, e.g.:

Conversation

andrerom commented Sep 8, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Solves:

BC

Testing

Todo

Future

Uh oh!

bdunogier left a comment

Choose a reason for hiding this comment

Uh oh!

bdunogier Sep 14, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

andrerom Nov 12, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bdunogier left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bdunogier Nov 24, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bdunogier Jan 16, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

andrerom commented Sep 8, 2016 •

edited

Loading

bdunogier Sep 14, 2016 •

edited

Loading

andrerom Nov 12, 2016 •

edited

Loading

bdunogier Nov 24, 2016 •

edited

Loading

bdunogier Jan 16, 2017 •

edited

Loading

andrerom commented Nov 24, 2016 •

edited

Loading