Reserve nodes when response has Cache-Control=private or Set-Cookie in CacheEnforcementVisitor

In the CacheEnforcementVisitor, reserve nodes whose response is either StrictNoCache, or has a Set-Cookie header, and do not trigger a fetch for these. Changed the flow to not pre-fetch error responses, since error responses are already set to have a ttl of 30 secs. Also, do not pre-fetch resources with a max-age of 0 since they will not be usable by the time the actual request comes in.

Also, the HttpResponse class returns -1 for cache expiration time for any strict no-cache resource. Since we want to know when such responses expire, adding the property 'shindig.cache.http.negativeCacheTtl'(also used in AbstractHttpCache), which if less than zero retains original behavior(of expiration time -1), and otherwise returns the actual expiration time of such resources.

[nikhilmadan23, 2011-02-09 11:21:55]

Readability fixes

[satya3656, 2011-02-10 05:43:54]

Updated the description for Cache-Control=max-age=0 case also

http://codereview.appspot.com/4001053/diff/21001/java/gadgets/src/main/java/o...
File
java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/CacheEnforcementVisitor.java
(left):

http://codereview.appspot.com/4001053/diff/21001/java/gadgets/src/main/java/o...
java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/CacheEnforcementVisitor.java:101:
// Also, investigate when Cache-Control is max-age=0. We are currently
triggering a fetch in
Also add checks for reserving in this case.

http://codereview.appspot.com/4001053/diff/21001/java/gadgets/src/main/java/o...
File
java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/CacheEnforcementVisitor.java
(right):

http://codereview.appspot.com/4001053/diff/21001/java/gadgets/src/main/java/o...
java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/CacheEnforcementVisitor.java:103:
// If the response is strict no-cache, or has a Set-Cookie header, reserve the
node. Do not
Update the comment, saying that prefetching of these kind resources is useless
as they should not be cached.

[nikhilmadan23, 2011-02-10 06:36:51]

http://codereview.appspot.com/4001053/diff/21001/java/gadgets/src/main/java/o...
File
java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/CacheEnforcementVisitor.java
(left):

http://codereview.appspot.com/4001053/diff/21001/java/gadgets/src/main/java/o...
java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/CacheEnforcementVisitor.java:101:
// Also, investigate when Cache-Control is max-age=0. We are currently
triggering a fetch in
On 2011/02/10 05:43:54, satya3656 wrote:
> Also add checks for reserving in this case.

Done.

http://codereview.appspot.com/4001053/diff/21001/java/gadgets/src/main/java/o...
File
java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/CacheEnforcementVisitor.java
(right):

http://codereview.appspot.com/4001053/diff/21001/java/gadgets/src/main/java/o...
java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/CacheEnforcementVisitor.java:103:
// If the response is strict no-cache, or has a Set-Cookie header, reserve the
node. Do not
On 2011/02/10 05:43:54, satya3656 wrote:
> Update the comment, saying that prefetching of these kind resources is useless
> as they should not be cached.

Done.

[satya3656, 2011-02-10 12:55:22]

LGTM minor comments

Add in the CacheEnforcementVisitor Class description, that you need to set the
strictNoCacheResorceTtl to a value > 0 if you want to use this class.

http://codereview.appspot.com/4001053/diff/27001/java/gadgets/src/main/java/o...
File
java/gadgets/src/main/java/org/apache/shindig/gadgets/http/HttpResponse.java
(right):

http://codereview.appspot.com/4001053/diff/27001/java/gadgets/src/main/java/o...
java/gadgets/src/main/java/org/apache/shindig/gadgets/http/HttpResponse.java:21:
import com.google.common.collect.*;
expand *

http://codereview.appspot.com/4001053/diff/27001/java/gadgets/src/main/java/o...
java/gadgets/src/main/java/org/apache/shindig/gadgets/http/HttpResponse.java:142:
private static long strictNoCacheResourceTtl =
DEFAULT_STRICT_NO_CACHE_RESOURCE_TTL;
Add to patch description why you need this.

http://codereview.appspot.com/4001053/diff/27001/java/gadgets/src/main/java/o...
File
java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/CacheEnforcementVisitor.java
(right):

http://codereview.appspot.com/4001053/diff/27001/java/gadgets/src/main/java/o...
java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/CacheEnforcementVisitor.java:118:
// ttl for such resources to 30secs, we should not keep pre-fetching these till
they become
30 seconds

http://codereview.appspot.com/4001053/diff/27001/java/gadgets/src/test/java/o...
File
java/gadgets/src/test/java/org/apache/shindig/gadgets/rewrite/CacheEnforcementVisitorTest.java
(right):

http://codereview.appspot.com/4001053/diff/27001/java/gadgets/src/test/java/o...
java/gadgets/src/test/java/org/apache/shindig/gadgets/rewrite/CacheEnforcementVisitorTest.java:20:
import com.google.common.collect.Maps;
indent

http://codereview.appspot.com/4001053/diff/27001/java/gadgets/src/test/java/o...
java/gadgets/src/test/java/org/apache/shindig/gadgets/rewrite/CacheEnforcementVisitorTest.java:35:
import static org.easymock.EasyMock.createMock;
move statics to top

[nikhilmadan23, 2011-02-10 13:20:24]

http://codereview.appspot.com/4001053/diff/27001/java/gadgets/src/main/java/o...
File
java/gadgets/src/main/java/org/apache/shindig/gadgets/http/HttpResponse.java
(right):

http://codereview.appspot.com/4001053/diff/27001/java/gadgets/src/main/java/o...
java/gadgets/src/main/java/org/apache/shindig/gadgets/http/HttpResponse.java:21:
import com.google.common.collect.*;
On 2011/02/10 12:55:22, satya3656 wrote:
> expand *

Done.

http://codereview.appspot.com/4001053/diff/27001/java/gadgets/src/main/java/o...
java/gadgets/src/main/java/org/apache/shindig/gadgets/http/HttpResponse.java:142:
private static long strictNoCacheResourceTtl =
DEFAULT_STRICT_NO_CACHE_RESOURCE_TTL;
On 2011/02/10 12:55:22, satya3656 wrote:
> Add to patch description why you need this.

Done.

http://codereview.appspot.com/4001053/diff/27001/java/gadgets/src/main/java/o...
File
java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/CacheEnforcementVisitor.java
(right):

http://codereview.appspot.com/4001053/diff/27001/java/gadgets/src/main/java/o...
java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/CacheEnforcementVisitor.java:118:
// ttl for such resources to 30secs, we should not keep pre-fetching these till
they become
On 2011/02/10 12:55:22, satya3656 wrote:
> 30 seconds

Done.

http://codereview.appspot.com/4001053/diff/27001/java/gadgets/src/test/java/o...
File
java/gadgets/src/test/java/org/apache/shindig/gadgets/rewrite/CacheEnforcementVisitorTest.java
(right):

http://codereview.appspot.com/4001053/diff/27001/java/gadgets/src/test/java/o...
java/gadgets/src/test/java/org/apache/shindig/gadgets/rewrite/CacheEnforcementVisitorTest.java:20:
import com.google.common.collect.Maps;
On 2011/02/10 12:55:22, satya3656 wrote:
> indent

Done.

http://codereview.appspot.com/4001053/diff/27001/java/gadgets/src/test/java/o...
java/gadgets/src/test/java/org/apache/shindig/gadgets/rewrite/CacheEnforcementVisitorTest.java:35:
import static org.easymock.EasyMock.createMock;
On 2011/02/10 12:55:22, satya3656 wrote:
> move statics to top

Done.

[gagan.goku, 2011-02-11 11:31:07]

LGTM

http://codereview.appspot.com/4001053/diff/29004/java/gadgets/src/main/java/o...
File
java/gadgets/src/main/java/org/apache/shindig/gadgets/http/HttpResponse.java
(right):

http://codereview.appspot.com/4001053/diff/29004/java/gadgets/src/main/java/o...
java/gadgets/src/main/java/org/apache/shindig/gadgets/http/HttpResponse.java:21:
import com.google.common.collect.ImmutableList;
might as well keep it .*, this just seems like unnecessary imports.

[gagan.goku, 2011-02-11 16:08:15]

Added wildcard for com.google.common.collect.* and removed extra-non-java-1.5
@Override's.
Build looks good.
Committed as r1069855.