You are viewing a plain text version of this content. The canonical link for it is here.

Posted to dev@shindig.apache.org by Stanton Sievers <si...@gmail.com> on 2013/05/31 21:49:23 UTC

Review Request: Don't force cache time-to-lives on responses with an error status

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/11584/
-----------------------------------------------------------

Review request for shindig.


Description
-------

org.apache.shindig.gadgets.DefaultGadgetSpecFactory and org.apache.shindig.gadgets.DefaultMessageBundleFactory utilize a property in shindig.properties (shindig.cache.xml.refreshInterval) to force the RequestPipeline/Fetcher layer to cache specs and message bundles for a set amount of time, regardless of the cache headers on the response.  This also forces the cache ttl in the case that the response is an error instead of using the "shindig.cache.http.negativeCacheTtl" value in shindig.properties, which defaults to one minute.

I'd rather we always use the negativeCacheTtl in this case and ignore the forced cache ttl.

This patch addresses that.  Do others think this is a good idea?


Diffs
-----

  http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/http/AbstractHttpCache.java 1485016 
  http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/test/java/org/apache/shindig/gadgets/http/AbstractHttpCacheTest.java 1485016 

Diff: https://reviews.apache.org/r/11584/diff/


Testing
-------

I functionally tested this and also wrote a unit test.  All existing unit tests continue to pass.


Thanks,

Stanton Sievers

Re: Review Request: Don't force cache time-to-lives on responses with an error status

Posted by Stanton Sievers <si...@gmail.com>.


> On May 31, 2013, 9:38 p.m., Ryan Baxter wrote:
> > http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/http/AbstractHttpCache.java, line 120
> > <https://reviews.apache.org/r/11584/diff/1/?file=299651#file299651line120>
> >
> >     Will the negative cache ttl value already be set on the response at this point?

No.  It's done dynamically when you call HttpResponse.isStale() to see if the response is expired.  You can see the details further down the call hierarchy from isStale() in HttpResponse.getCacheExpiration().  It ends up that in the case of 401 or 403s, by default, we'll obey the cache-control headers and not use the negative cache ttl.


- Stanton


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/11584/#review21272
-----------------------------------------------------------


On May 31, 2013, 7:49 p.m., Stanton Sievers wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/11584/
> -----------------------------------------------------------
> 
> (Updated May 31, 2013, 7:49 p.m.)
> 
> 
> Review request for shindig.
> 
> 
> Description
> -------
> 
> org.apache.shindig.gadgets.DefaultGadgetSpecFactory and org.apache.shindig.gadgets.DefaultMessageBundleFactory utilize a property in shindig.properties (shindig.cache.xml.refreshInterval) to force the RequestPipeline/Fetcher layer to cache specs and message bundles for a set amount of time, regardless of the cache headers on the response.  This also forces the cache ttl in the case that the response is an error instead of using the "shindig.cache.http.negativeCacheTtl" value in shindig.properties, which defaults to one minute.
> 
> I'd rather we always use the negativeCacheTtl in this case and ignore the forced cache ttl.
> 
> This patch addresses that.  Do others think this is a good idea?
> 
> 
> Diffs
> -----
> 
>   http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/http/AbstractHttpCache.java 1485016 
>   http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/test/java/org/apache/shindig/gadgets/http/AbstractHttpCacheTest.java 1485016 
> 
> Diff: https://reviews.apache.org/r/11584/diff/
> 
> 
> Testing
> -------
> 
> I functionally tested this and also wrote a unit test.  All existing unit tests continue to pass.
> 
> 
> Thanks,
> 
> Stanton Sievers
> 
>

Re: Review Request: Don't force cache time-to-lives on responses with an error status

Posted by Ryan Baxter <rb...@gmail.com>.

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/11584/#review21272
-----------------------------------------------------------



http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/http/AbstractHttpCache.java
<https://reviews.apache.org/r/11584/#comment44147>

    Will the negative cache ttl value already be set on the response at this point?


- Ryan Baxter


On May 31, 2013, 7:49 p.m., Stanton Sievers wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/11584/
> -----------------------------------------------------------
> 
> (Updated May 31, 2013, 7:49 p.m.)
> 
> 
> Review request for shindig.
> 
> 
> Description
> -------
> 
> org.apache.shindig.gadgets.DefaultGadgetSpecFactory and org.apache.shindig.gadgets.DefaultMessageBundleFactory utilize a property in shindig.properties (shindig.cache.xml.refreshInterval) to force the RequestPipeline/Fetcher layer to cache specs and message bundles for a set amount of time, regardless of the cache headers on the response.  This also forces the cache ttl in the case that the response is an error instead of using the "shindig.cache.http.negativeCacheTtl" value in shindig.properties, which defaults to one minute.
> 
> I'd rather we always use the negativeCacheTtl in this case and ignore the forced cache ttl.
> 
> This patch addresses that.  Do others think this is a good idea?
> 
> 
> Diffs
> -----
> 
>   http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/http/AbstractHttpCache.java 1485016 
>   http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/test/java/org/apache/shindig/gadgets/http/AbstractHttpCacheTest.java 1485016 
> 
> Diff: https://reviews.apache.org/r/11584/diff/
> 
> 
> Testing
> -------
> 
> I functionally tested this and also wrote a unit test.  All existing unit tests continue to pass.
> 
> 
> Thanks,
> 
> Stanton Sievers
> 
>

Re: Review Request: Don't force cache time-to-lives on responses with an error status

Posted by Stanton Sievers <si...@gmail.com>.

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/11584/#review21477
-----------------------------------------------------------


If there are no other concerns, I'll commit this in ~24 hours. Thanks.

- Stanton Sievers


On June 3, 2013, 8:29 p.m., Stanton Sievers wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/11584/
> -----------------------------------------------------------
> 
> (Updated June 3, 2013, 8:29 p.m.)
> 
> 
> Review request for shindig.
> 
> 
> Description
> -------
> 
> org.apache.shindig.gadgets.DefaultGadgetSpecFactory and org.apache.shindig.gadgets.DefaultMessageBundleFactory utilize a property in shindig.properties (shindig.cache.xml.refreshInterval) to force the RequestPipeline/Fetcher layer to cache specs and message bundles for a set amount of time, regardless of the cache headers on the response.  This also forces the cache ttl in the case that the response is an error instead of using the "shindig.cache.http.negativeCacheTtl" value in shindig.properties, which defaults to one minute.
> 
> I'd rather we always use the negativeCacheTtl in this case and ignore the forced cache ttl.
> 
> This patch addresses that.  Do others think this is a good idea?
> 
> 
> This addresses bug SHINDIG-1920.
>     https://issues.apache.org/jira/browse/SHINDIG-1920
> 
> 
> Diffs
> -----
> 
>   http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/http/AbstractHttpCache.java 1485016 
>   http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/http/HttpResponse.java 1485016 
>   http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/test/java/org/apache/shindig/gadgets/http/AbstractHttpCacheTest.java 1485016 
>   http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/test/java/org/apache/shindig/gadgets/http/HttpResponseTest.java 1485016 
> 
> Diff: https://reviews.apache.org/r/11584/diff/
> 
> 
> Testing
> -------
> 
> I functionally tested this and also wrote a unit test.  All existing unit tests continue to pass.
> 
> 
> Thanks,
> 
> Stanton Sievers
> 
>

Re: Review Request: Don't force cache time-to-lives on responses with an error status

Posted by Dan Dumont <dd...@us.ibm.com>.

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/11584/#review21404
-----------------------------------------------------------

Ship it!


Ship It!

- Dan Dumont


On June 3, 2013, 8:29 p.m., Stanton Sievers wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/11584/
> -----------------------------------------------------------
> 
> (Updated June 3, 2013, 8:29 p.m.)
> 
> 
> Review request for shindig.
> 
> 
> Description
> -------
> 
> org.apache.shindig.gadgets.DefaultGadgetSpecFactory and org.apache.shindig.gadgets.DefaultMessageBundleFactory utilize a property in shindig.properties (shindig.cache.xml.refreshInterval) to force the RequestPipeline/Fetcher layer to cache specs and message bundles for a set amount of time, regardless of the cache headers on the response.  This also forces the cache ttl in the case that the response is an error instead of using the "shindig.cache.http.negativeCacheTtl" value in shindig.properties, which defaults to one minute.
> 
> I'd rather we always use the negativeCacheTtl in this case and ignore the forced cache ttl.
> 
> This patch addresses that.  Do others think this is a good idea?
> 
> 
> This addresses bug SHINDIG-1920.
>     https://issues.apache.org/jira/browse/SHINDIG-1920
> 
> 
> Diffs
> -----
> 
>   http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/http/AbstractHttpCache.java 1485016 
>   http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/http/HttpResponse.java 1485016 
>   http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/test/java/org/apache/shindig/gadgets/http/AbstractHttpCacheTest.java 1485016 
>   http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/test/java/org/apache/shindig/gadgets/http/HttpResponseTest.java 1485016 
> 
> Diff: https://reviews.apache.org/r/11584/diff/
> 
> 
> Testing
> -------
> 
> I functionally tested this and also wrote a unit test.  All existing unit tests continue to pass.
> 
> 
> Thanks,
> 
> Stanton Sievers
> 
>

Re: Review Request: Don't force cache time-to-lives on responses with an error status

Posted by Stanton Sievers <si...@gmail.com>.

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/11584/#review21519
-----------------------------------------------------------

Ship it!


Committed revision 1490276.

- Stanton Sievers


On June 3, 2013, 8:29 p.m., Stanton Sievers wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/11584/
> -----------------------------------------------------------
> 
> (Updated June 3, 2013, 8:29 p.m.)
> 
> 
> Review request for shindig.
> 
> 
> Description
> -------
> 
> org.apache.shindig.gadgets.DefaultGadgetSpecFactory and org.apache.shindig.gadgets.DefaultMessageBundleFactory utilize a property in shindig.properties (shindig.cache.xml.refreshInterval) to force the RequestPipeline/Fetcher layer to cache specs and message bundles for a set amount of time, regardless of the cache headers on the response.  This also forces the cache ttl in the case that the response is an error instead of using the "shindig.cache.http.negativeCacheTtl" value in shindig.properties, which defaults to one minute.
> 
> I'd rather we always use the negativeCacheTtl in this case and ignore the forced cache ttl.
> 
> This patch addresses that.  Do others think this is a good idea?
> 
> 
> This addresses bug SHINDIG-1920.
>     https://issues.apache.org/jira/browse/SHINDIG-1920
> 
> 
> Diffs
> -----
> 
>   http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/http/AbstractHttpCache.java 1485016 
>   http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/http/HttpResponse.java 1485016 
>   http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/test/java/org/apache/shindig/gadgets/http/AbstractHttpCacheTest.java 1485016 
>   http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/test/java/org/apache/shindig/gadgets/http/HttpResponseTest.java 1485016 
> 
> Diff: https://reviews.apache.org/r/11584/diff/
> 
> 
> Testing
> -------
> 
> I functionally tested this and also wrote a unit test.  All existing unit tests continue to pass.
> 
> 
> Thanks,
> 
> Stanton Sievers
> 
>

Re: Review Request: Don't force cache time-to-lives on responses with an error status

Posted by Stanton Sievers <si...@gmail.com>.

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/11584/
-----------------------------------------------------------

(Updated June 3, 2013, 8:29 p.m.)

Review request for shindig.

Changes
-------

Updating the patch to include a use case I missed the first time around. We run into the new use case when one of the response's status code is in the negative cache exempt list, e.g., is 401 or 403 AND the response has not cache control headers. In this case we would have fallen through to the end of the getCacheExpiration method and would have returned a the default TTL and not the default negative TTL. This would have resulted in 401 and 403s being cached for much longer than we want.

The updated patch fixes this and adds a unit test for this case.

Description
-------

org.apache.shindig.gadgets.DefaultGadgetSpecFactory and org.apache.shindig.gadgets.DefaultMessageBundleFactory utilize a property in shindig.properties (shindig.cache.xml.refreshInterval) to force the RequestPipeline/Fetcher layer to cache specs and message bundles for a set amount of time, regardless of the cache headers on the response. This also forces the cache ttl in the case that the response is an error instead of using the "shindig.cache.http.negativeCacheTtl" value in shindig.properties, which defaults to one minute.

I'd rather we always use the negativeCacheTtl in this case and ignore the forced cache ttl.

This patch addresses that. Do others think this is a good idea?

This addresses bug SHINDIG-1920.
https://issues.apache.org/jira/browse/SHINDIG-1920

Diffs (updated)
-----

http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/http/AbstractHttpCache.java 1485016
http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/http/HttpResponse.java 1485016
http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/test/java/org/apache/shindig/gadgets/http/AbstractHttpCacheTest.java 1485016
http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/test/java/org/apache/shindig/gadgets/http/HttpResponseTest.java 1485016

Diff: https://reviews.apache.org/r/11584/diff/

Testing
-------

I functionally tested this and also wrote a unit test. All existing unit tests continue to pass.

Thanks,

Stanton Sievers

Re: Review Request: Don't force cache time-to-lives on responses with an error status

Posted by Stanton Sievers <si...@gmail.com>.

(Updated June 2, 2013, 1:35 p.m.)

Review request for shindig.

Changes
-------

Adding a JIRA

Description
-------

I'd rather we always use the negativeCacheTtl in this case and ignore the forced cache ttl.

This patch addresses that. Do others think this is a good idea?

This addresses bug SHINDIG-1920.
https://issues.apache.org/jira/browse/SHINDIG-1920

Diffs
-----

Diff: https://reviews.apache.org/r/11584/diff/

Testing
-------

I functionally tested this and also wrote a unit test. All existing unit tests continue to pass.

Thanks,

Stanton Sievers

Re: Review Request: Don't force cache time-to-lives on responses with an error status

Posted by Dan Dumont <dd...@us.ibm.com>.

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/11584/#review21269
-----------------------------------------------------------

Ship it!


I agree with this approach.

- Dan Dumont


On May 31, 2013, 7:49 p.m., Stanton Sievers wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/11584/
> -----------------------------------------------------------
> 
> (Updated May 31, 2013, 7:49 p.m.)
> 
> 
> Review request for shindig.
> 
> 
> Description
> -------
> 
> org.apache.shindig.gadgets.DefaultGadgetSpecFactory and org.apache.shindig.gadgets.DefaultMessageBundleFactory utilize a property in shindig.properties (shindig.cache.xml.refreshInterval) to force the RequestPipeline/Fetcher layer to cache specs and message bundles for a set amount of time, regardless of the cache headers on the response.  This also forces the cache ttl in the case that the response is an error instead of using the "shindig.cache.http.negativeCacheTtl" value in shindig.properties, which defaults to one minute.
> 
> I'd rather we always use the negativeCacheTtl in this case and ignore the forced cache ttl.
> 
> This patch addresses that.  Do others think this is a good idea?
> 
> 
> Diffs
> -----
> 
>   http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/http/AbstractHttpCache.java 1485016 
>   http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/test/java/org/apache/shindig/gadgets/http/AbstractHttpCacheTest.java 1485016 
> 
> Diff: https://reviews.apache.org/r/11584/diff/
> 
> 
> Testing
> -------
> 
> I functionally tested this and also wrote a unit test.  All existing unit tests continue to pass.
> 
> 
> Thanks,
> 
> Stanton Sievers
> 
>

Re: Review Request: Don't force cache time-to-lives on responses with an error status

Posted by Ryan Baxter <rb...@gmail.com>.

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/11584/#review21277
-----------------------------------------------------------

Ship it!



http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/http/AbstractHttpCache.java
<https://reviews.apache.org/r/11584/#comment44150>

    Ok that is why I was asking.  I saw the code in HttpResponse that excluded 401s and 403s.  


- Ryan Baxter


On May 31, 2013, 7:49 p.m., Stanton Sievers wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/11584/
> -----------------------------------------------------------
> 
> (Updated May 31, 2013, 7:49 p.m.)
> 
> 
> Review request for shindig.
> 
> 
> Description
> -------
> 
> org.apache.shindig.gadgets.DefaultGadgetSpecFactory and org.apache.shindig.gadgets.DefaultMessageBundleFactory utilize a property in shindig.properties (shindig.cache.xml.refreshInterval) to force the RequestPipeline/Fetcher layer to cache specs and message bundles for a set amount of time, regardless of the cache headers on the response.  This also forces the cache ttl in the case that the response is an error instead of using the "shindig.cache.http.negativeCacheTtl" value in shindig.properties, which defaults to one minute.
> 
> I'd rather we always use the negativeCacheTtl in this case and ignore the forced cache ttl.
> 
> This patch addresses that.  Do others think this is a good idea?
> 
> 
> Diffs
> -----
> 
>   http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/http/AbstractHttpCache.java 1485016 
>   http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/test/java/org/apache/shindig/gadgets/http/AbstractHttpCacheTest.java 1485016 
> 
> Diff: https://reviews.apache.org/r/11584/diff/
> 
> 
> Testing
> -------
> 
> I functionally tested this and also wrote a unit test.  All existing unit tests continue to pass.
> 
> 
> Thanks,
> 
> Stanton Sievers
> 
>