have flush return the count of successful invalidation requests #47

dbu · 2014-03-09T20:09:10Z

as discussed in https://github.com/ddeboer/FOSHttpCacheBundle/pull/38/files#r10415347

dbu · 2014-03-09T20:09:40Z

src/Invalidation/AbstractCacheProxy.php

@@ -106,11 +106,12 @@ public function flush()
    {
        $queue = $this->queue;
        if (0 === count($queue)) {
-            return;
+            return 0;


note that this is no real BC break as we did not respect the fluent interface we promised in the interface...

dbu · 2014-03-09T20:14:13Z

hm, travis fails but i think this reveals we have an issue here. failing because something was not cached makes no sense - the application can't know if the document is cached or not.

do we want to ignore 404 errors or do we want to change our propsed varnish configuration? probably a 204 No Content response code would make most sense.

ddeboer · 2014-03-09T21:30:58Z

tests/Functional/VarnishTest.php

+                ->purge('/a')
+                ->flush()
+        );
+    }


Does this need to be a functional rather than a unit test?

ddeboer · 2014-03-09T21:36:18Z

probably a 204 No Content response code would make most sense.

Yeah, please change that in purge.vcl and the docs. I can’t really think of a use case where the application needs to know whether the content that should be invalidated was cached or not.

ddeboer · 2014-03-09T21:41:47Z

src/Invalidation/AbstractCacheProxy.php

@@ -167,7 +169,10 @@ protected function sendRequests(array $requests)
            $this->client->send($allRequests);
        } catch (MultiTransferException $e) {
            $this->handleException($e);
+            $count = $e->getSuccessfulRequests();


This conflates the two meanings of “number of requests”: now $count no longer means “number of requests sent to each proxy” but “number of (successful) requests sent in total to all proxies together”.

ouch, you are right. hm, i guess this will become very difficult to handle. what should we do? return -1 if there is at least one success, to indicate the number could not be determined due to a partial error? we could divide by the number of backends but that is a wild guess...

Or simply change the statistic to mean “total number of (successful) requests”? Would that make it less useful?

trying to build a test for this helped - it just can't matter, as handleException throws the retransformed exception and we never return a count for a (partially) failed invalidation action.

dbu · 2014-03-09T22:18:54Z

tests/Functional/Fixtures/varnish/purge.vcl

    }
 }

 sub vcl_miss {
    if (req.request == "PURGE") {
        purge;
-        error 404 "Not in cache";
+        error 204 "Purged (Not in cache)";


like this its still debuggable what is going on.

dbu · 2014-03-09T22:21:22Z

okay, updated. i think now it makes sense. we probably want to handle the exceptions in the symfony console listener and print some information that cache invalidation did not work out. (if no invalidation was sent, i would expect to have no exception ever, so that should be correct to report when something went wrong)

ddeboer · 2014-03-09T22:23:05Z

src/Invalidation/AbstractCacheProxy.php

@@ -168,6 +170,8 @@ protected function sendRequests(array $requests)
        } catch (MultiTransferException $e) {
            $this->handleException($e);
        }
+
+        return $count;


No need for setting a variable anymore. Just return count($requests) directly.

ddeboer · 2014-03-09T22:25:12Z

One minor detail, looks fine otherwise.

dbu · 2014-03-09T22:28:00Z

removed the $count variable.

ddeboer · 2014-03-10T08:45:36Z

tests/Unit/Invalidation/VarnishTest.php

@@ -6,6 +6,7 @@
 use FOS\HttpCache\Invalidation\Varnish;
 use Guzzle\Http\Client;
 use Guzzle\Http\Exception\CurlException;
+use Guzzle\Http\Exception\MultiTransferException;


Looks like this is not being used.

you are right. updated.

dbu · 2014-03-11T07:23:00Z

simplified and squashed.

have flush return the count of successful invalidation requests

ddeboer · 2014-03-11T09:49:50Z

Thanks!

dbu reviewed Mar 9, 2014
View reviewed changes

ddeboer reviewed Mar 9, 2014
View reviewed changes

tests/Functional/VarnishTest.php

->purge('/a')

->flush()

);

}

Copy link

Member

ddeboer Mar 9, 2014

Choose a reason for hiding this comment

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

Does this need to be a functional rather than a unit test?

ddeboer reviewed Mar 9, 2014
View reviewed changes

dbu mentioned this pull request Mar 9, 2014

Flush cache manager at command termination (fix #32) FriendsOfSymfony/FOSHttpCacheBundle#38

Merged

dbu reviewed Mar 9, 2014
View reviewed changes

ddeboer reviewed Mar 9, 2014
View reviewed changes

ddeboer reviewed Mar 10, 2014
View reviewed changes

have flush return the count of successful invalidation requests

99a6717

ddeboer added a commit that referenced this pull request Mar 11, 2014

Merge pull request #47 from FriendsOfSymfony/flush-return-count

6cbd607

have flush return the count of successful invalidation requests

ddeboer merged commit 6cbd607 into master Mar 11, 2014

ddeboer deleted the flush-return-count branch March 11, 2014 09:49

have flush return the count of successful invalidation requests #47

have flush return the count of successful invalidation requests #47

Uh oh!

Conversation

dbu commented Mar 9, 2014

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dbu commented Mar 9, 2014

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ddeboer commented Mar 9, 2014

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!

dbu commented Mar 9, 2014

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ddeboer commented Mar 9, 2014

Uh oh!

dbu commented Mar 9, 2014

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dbu commented Mar 11, 2014

Uh oh!

ddeboer commented Mar 11, 2014

Uh oh!

Uh oh!