Issue 4047043: Storing Strict No-Cache responses in AbstractHttpCache (Closed)
|
Created:
15 years, 1 month ago by nikhilmadan23 Modified:
15 years, 1 month ago Base URL:
http://svn.apache.org/repos/asf/shindig/trunk/ Visibility:
Public. |
DescriptionThe CacheEnforcementVisitor triggers fetches for resources on a page which are not in cache. Since "strict no-cache" resources are never cached, fetches will be triggered for such resources each time. Therefore, for such responses, we add support to the AbstractHttpCache to store the Cache-Control headers(no content, no cookies or other headers). Such resources are stored with a max-age of 1 day.
Patch Set 1 #Patch Set 2 : Removed extra newlines #Patch Set 3 : Removed some new lines #
Total comments: 12
Patch Set 4 : Addressing Anupama's comments #Patch Set 5 : Minor changes #
Total comments: 4
Patch Set 6 : Addressing Anupama's comments and changes to buildStrictNoCacheHttpResponse #Patch Set 7 : Error in uploading last patch(incomplete) #
Total comments: 10
Patch Set 8 : Addressing Anupama's comments #Patch Set 9 : Minor changes #Patch Set 10 : Fixing Typos #Patch Set 11 : Reuploading #Patch Set 12 : Another typo #Patch Set 13 : Fixing Chunk Mismatch #Patch Set 14 : Fixing Chunk Mismatch #
Total comments: 1
Patch Set 15 : Removing added APIs #
Total comments: 1
Patch Set 16 : Refactoring #
Total comments: 6
Patch Set 17 : Addressing comments #Patch Set 18 : Removing unecessary code #
Total comments: 2
Patch Set 19 : Make getStrictNoCacheTtl a property in shindig instead #Patch Set 20 : Changed binding code in AbstractHttpCacheTest.java #
Total comments: 2
Patch Set 21 : Addressing comments #
Total comments: 12
Patch Set 22 : Addressing comments #
Total comments: 2
Patch Set 23 : Addressing comments #Patch Set 24 : removing space #MessagesTotal messages: 33
Removed extra newlines
Sign in to reply to this message.
Removed some new lines
Sign in to reply to this message.
http://codereview.appspot.com/4047043/diff/13001/java/gadgets/src/main/java/o... File java/gadgets/src/main/java/org/apache/shindig/gadgets/http/AbstractHttpCache.java (right): http://codereview.appspot.com/4047043/diff/13001/java/gadgets/src/main/java/o... java/gadgets/src/main/java/org/apache/shindig/gadgets/http/AbstractHttpCache.java:48: HttpResponse response = getResponseIgnoringStrictHeaders(request); Is it better to use getResponseIgnoringStrictNoCacheHeaders? http://codereview.appspot.com/4047043/diff/13001/java/gadgets/src/main/java/o... java/gadgets/src/main/java/org/apache/shindig/gadgets/http/AbstractHttpCache.java:49: if (response!= null && !response.isStrictNoCache()) { Space before != http://codereview.appspot.com/4047043/diff/13001/java/gadgets/src/main/java/o... java/gadgets/src/main/java/org/apache/shindig/gadgets/http/AbstractHttpCache.java:56: * Returns a response from cache, including any strict no-cache responses End comments with periods. http://codereview.appspot.com/4047043/diff/13001/java/gadgets/src/main/java/o... java/gadgets/src/main/java/org/apache/shindig/gadgets/http/AbstractHttpCache.java:78: if (isCacheable(request) && !isResponseStatusNotModified(response)) { Why do we need this new check for !isResponseStatusNotModified now? Does this not change the behavior for existing usages of cacheable responses when addResponse is called on these? http://codereview.appspot.com/4047043/diff/13001/java/gadgets/src/main/java/o... java/gadgets/src/main/java/org/apache/shindig/gadgets/http/AbstractHttpCache.java:82: } else { Indentation off. http://codereview.appspot.com/4047043/diff/13001/java/gadgets/src/main/java/o... File java/gadgets/src/main/java/org/apache/shindig/gadgets/http/HttpResponseBuilder.java (right): http://codereview.appspot.com/4047043/diff/13001/java/gadgets/src/main/java/o... java/gadgets/src/main/java/org/apache/shindig/gadgets/http/HttpResponseBuilder.java:239: ArrayList<String> directives = new ArrayList<String>(); Is it possible to use String[] as currently done in HttpResponse::getCacheControlMaxAge? That seems to need shorter syntax on each line :) Also, there is a trim() called on directives in that method which we too might need.
Sign in to reply to this message.
http://codereview.appspot.com/4047043/diff/13001/java/gadgets/src/main/java/o... File java/gadgets/src/main/java/org/apache/shindig/gadgets/http/AbstractHttpCache.java (right): http://codereview.appspot.com/4047043/diff/13001/java/gadgets/src/main/java/o... java/gadgets/src/main/java/org/apache/shindig/gadgets/http/AbstractHttpCache.java:48: HttpResponse response = getResponseIgnoringStrictHeaders(request); On 2011/01/20 09:20:50, anupama.dutta wrote: > Is it better to use getResponseIgnoringStrictNoCacheHeaders? Done. http://codereview.appspot.com/4047043/diff/13001/java/gadgets/src/main/java/o... java/gadgets/src/main/java/org/apache/shindig/gadgets/http/AbstractHttpCache.java:49: if (response!= null && !response.isStrictNoCache()) { On 2011/01/20 09:20:50, anupama.dutta wrote: > Space before != Done. http://codereview.appspot.com/4047043/diff/13001/java/gadgets/src/main/java/o... java/gadgets/src/main/java/org/apache/shindig/gadgets/http/AbstractHttpCache.java:56: * Returns a response from cache, including any strict no-cache responses On 2011/01/20 09:20:50, anupama.dutta wrote: > End comments with periods. Done. http://codereview.appspot.com/4047043/diff/13001/java/gadgets/src/main/java/o... java/gadgets/src/main/java/org/apache/shindig/gadgets/http/AbstractHttpCache.java:78: if (isCacheable(request) && !isResponseStatusNotModified(response)) { On 2011/01/20 09:20:50, anupama.dutta wrote: > Why do we need this new check for !isResponseStatusNotModified now? Does this > not change the behavior for existing usages of cacheable responses when > addResponse is called on these? addResponse calls isCacheable(HttpRequest, HttpResponse) which checks that the response is not a 304, and not strict no-cache. We need to ensure that calls through addResponseIgnoringStrictNoCacheHeaders also don't cache 304s. Thats why I thought it'd be better if we check it here as well. So this shouldn't effect the existing behavior, though this means that the 304 check is done twice. I considered removing this check from isCacheable(HttpRequest, HttpResponse) but that might change the behavior of existing child classes. http://codereview.appspot.com/4047043/diff/13001/java/gadgets/src/main/java/o... java/gadgets/src/main/java/org/apache/shindig/gadgets/http/AbstractHttpCache.java:82: } else { On 2011/01/20 09:20:50, anupama.dutta wrote: > Indentation off. Done. http://codereview.appspot.com/4047043/diff/13001/java/gadgets/src/main/java/o... File java/gadgets/src/main/java/org/apache/shindig/gadgets/http/HttpResponseBuilder.java (right): http://codereview.appspot.com/4047043/diff/13001/java/gadgets/src/main/java/o... java/gadgets/src/main/java/org/apache/shindig/gadgets/http/HttpResponseBuilder.java:239: ArrayList<String> directives = new ArrayList<String>(); On 2011/01/20 09:20:50, anupama.dutta wrote: > Is it possible to use String[] as currently done in > HttpResponse::getCacheControlMaxAge? That seems to need shorter syntax on each > line :) Also, there is a trim() called on directives in that method which we too > might need. I've added the trim here. Thanks for pointing that out. As for the suggestion to use an Array, I find this easier since we don't have to check where cacheControl is null and we can add/delete elements from it. The code I wrote using arrays was a few lines longer than this. What do you think?
Sign in to reply to this message.
http://codereview.appspot.com/4047043/diff/5002/java/gadgets/src/main/java/or... File java/gadgets/src/main/java/org/apache/shindig/gadgets/http/AbstractHttpCache.java (right): http://codereview.appspot.com/4047043/diff/5002/java/gadgets/src/main/java/or... java/gadgets/src/main/java/org/apache/shindig/gadgets/http/AbstractHttpCache.java:79: if (isCacheable(request) && !isResponseStatusNotModified(response)) { isCacheable(request, response) also has a check for returning true when request.getCacheTtl() != -1, irrespective of the response's value. Since our goal would be to have all checks except the final strictNoCache check in the original method, maybe we should just add a parameter to the isCacheable(request, response) method called "allowStrictNoCacheResponses" and set that param to true here so that the remaining 3 checks are done correctly?
Sign in to reply to this message.
http://codereview.appspot.com/4047043/diff/5002/java/gadgets/src/test/java/or... File java/gadgets/src/test/java/org/apache/shindig/gadgets/http/HttpResponseBuilderTest.java (right): http://codereview.appspot.com/4047043/diff/5002/java/gadgets/src/test/java/or... java/gadgets/src/test/java/org/apache/shindig/gadgets/http/HttpResponseBuilderTest.java:110: public void setCacheControlMaxAge() { Add 2 more tests with the following variations: - the added header has public, max-age=123, no-transform (with spaces in b/w). - the added header has public, max-age=12=ab (with 2 equals signs).
Sign in to reply to this message.
Addressing Anupama's comments and changes to buildStrictNoCacheHttpResponse
Sign in to reply to this message.
Error in uploading last patch(incomplete)
Sign in to reply to this message.
http://codereview.appspot.com/4047043/diff/5002/java/gadgets/src/main/java/or... File java/gadgets/src/main/java/org/apache/shindig/gadgets/http/AbstractHttpCache.java (right): http://codereview.appspot.com/4047043/diff/5002/java/gadgets/src/main/java/or... java/gadgets/src/main/java/org/apache/shindig/gadgets/http/AbstractHttpCache.java:79: if (isCacheable(request) && !isResponseStatusNotModified(response)) { On 2011/01/21 05:05:05, anupama.dutta wrote: > isCacheable(request, response) also has a check for returning true when > request.getCacheTtl() != -1, irrespective of the response's value. Since our > goal would be to have all checks except the final strictNoCache check in the > original method, maybe we should just add a parameter to the > isCacheable(request, response) method called "allowStrictNoCacheResponses" and > set that param to true here so that the remaining 3 checks are done correctly? Agreed. To avoid duplicated checks I've added a private method that finally adds the response to the cache. Please also look at the changes to buildStrictNoCacheHttpResponse regarding the pragma header and a few extra checks in the its tests. http://codereview.appspot.com/4047043/diff/5002/java/gadgets/src/test/java/or... File java/gadgets/src/test/java/org/apache/shindig/gadgets/http/HttpResponseBuilderTest.java (right): http://codereview.appspot.com/4047043/diff/5002/java/gadgets/src/test/java/or... java/gadgets/src/test/java/org/apache/shindig/gadgets/http/HttpResponseBuilderTest.java:110: public void setCacheControlMaxAge() { On 2011/01/21 05:15:27, anupama.dutta wrote: > Add 2 more tests with the following variations: > - the added header has public, max-age=123, no-transform (with spaces in b/w). > - the added header has public, max-age=12=ab (with 2 equals signs). Done. Please see the minor change in buildStrictNoCacheHttpResponse which removes spaces from the Cache-Control header.
Sign in to reply to this message.
http://codereview.appspot.com/4047043/diff/43001/java/gadgets/src/main/java/o... File java/gadgets/src/main/java/org/apache/shindig/gadgets/http/AbstractHttpCache.java (left): http://codereview.appspot.com/4047043/diff/43001/java/gadgets/src/main/java/o... java/gadgets/src/main/java/org/apache/shindig/gadgets/http/AbstractHttpCache.java:58: // Both are cacheable. Check for forced cache TTL overrides. Retain the // Both are cacheable. comment? http://codereview.appspot.com/4047043/diff/43001/java/gadgets/src/test/java/o... File java/gadgets/src/test/java/org/apache/shindig/gadgets/http/AbstractHttpCacheTest.java (right): http://codereview.appspot.com/4047043/diff/43001/java/gadgets/src/test/java/o... java/gadgets/src/test/java/org/apache/shindig/gadgets/http/AbstractHttpCacheTest.java:278: public void getResponseIgnoringStrictHeaders() { Since all these methods are duplicates of the existing tests for getResponse() how about adding these asserts directly in the existing tests? We will avoid duplicating a lot of code, and this will also ensure that for each existing getResponse test, we do test the equivalent behavior in getResponseIgnoringStrictCacheHeaders. http://codereview.appspot.com/4047043/diff/43001/java/gadgets/src/test/java/o... java/gadgets/src/test/java/org/apache/shindig/gadgets/http/AbstractHttpCacheTest.java:339: assertEquals(response, cache.map.get(key)); Can we assert addResponseIgnoringStrictHeaders behavior in all these tests/ http://codereview.appspot.com/4047043/diff/43001/java/gadgets/src/test/java/o... File java/gadgets/src/test/java/org/apache/shindig/gadgets/http/HttpResponseBuilderTest.java (right): http://codereview.appspot.com/4047043/diff/43001/java/gadgets/src/test/java/o... java/gadgets/src/test/java/org/apache/shindig/gadgets/http/HttpResponseBuilderTest.java:126: assertEquals("public,no-transform,max-age=12345", headers.get("Cache-Control").iterator().next()); 100 chars. http://codereview.appspot.com/4047043/diff/43001/java/gadgets/src/test/java/o... java/gadgets/src/test/java/org/apache/shindig/gadgets/http/HttpResponseBuilderTest.java:136: assertEquals("public,max-age=12=ab,max-age=12345", headers.get("Cache-Control").iterator().next()); 100 chars.
Sign in to reply to this message.
http://codereview.appspot.com/4047043/diff/43001/java/gadgets/src/main/java/o... File java/gadgets/src/main/java/org/apache/shindig/gadgets/http/AbstractHttpCache.java (left): http://codereview.appspot.com/4047043/diff/43001/java/gadgets/src/main/java/o... java/gadgets/src/main/java/org/apache/shindig/gadgets/http/AbstractHttpCache.java:58: // Both are cacheable. Check for forced cache TTL overrides. On 2011/01/24 14:44:07, anupama.dutta wrote: > Retain the // Both are cacheable. comment? Done. http://codereview.appspot.com/4047043/diff/43001/java/gadgets/src/test/java/o... File java/gadgets/src/test/java/org/apache/shindig/gadgets/http/AbstractHttpCacheTest.java (right): http://codereview.appspot.com/4047043/diff/43001/java/gadgets/src/test/java/o... java/gadgets/src/test/java/org/apache/shindig/gadgets/http/AbstractHttpCacheTest.java:278: public void getResponseIgnoringStrictHeaders() { On 2011/01/24 14:44:07, anupama.dutta wrote: > Since all these methods are duplicates of the existing tests for getResponse() > how about adding these asserts directly in the existing tests? We will avoid > duplicating a lot of code, and this will also ensure that for each existing > getResponse test, we do test the equivalent behavior in > getResponseIgnoringStrictCacheHeaders. Done. http://codereview.appspot.com/4047043/diff/43001/java/gadgets/src/test/java/o... java/gadgets/src/test/java/org/apache/shindig/gadgets/http/AbstractHttpCacheTest.java:339: assertEquals(response, cache.map.get(key)); On 2011/01/24 14:44:07, anupama.dutta wrote: > Can we assert addResponseIgnoringStrictHeaders behavior in all these tests/ Done. http://codereview.appspot.com/4047043/diff/43001/java/gadgets/src/test/java/o... File java/gadgets/src/test/java/org/apache/shindig/gadgets/http/HttpResponseBuilderTest.java (right): http://codereview.appspot.com/4047043/diff/43001/java/gadgets/src/test/java/o... java/gadgets/src/test/java/org/apache/shindig/gadgets/http/HttpResponseBuilderTest.java:126: assertEquals("public,no-transform,max-age=12345", headers.get("Cache-Control").iterator().next()); On 2011/01/24 14:44:07, anupama.dutta wrote: > 100 chars. Done. http://codereview.appspot.com/4047043/diff/43001/java/gadgets/src/test/java/o... java/gadgets/src/test/java/org/apache/shindig/gadgets/http/HttpResponseBuilderTest.java:136: assertEquals("public,max-age=12=ab,max-age=12345", headers.get("Cache-Control").iterator().next()); On 2011/01/24 14:44:07, anupama.dutta wrote: > 100 chars. Done.
Sign in to reply to this message.
Fixing Typos
Sign in to reply to this message.
LGTM. Please send out to dev@ with Gagan and Fargo as reviewers.
Sign in to reply to this message.
I'd prefer to see this functionality provided in configurable fashion rather
than by introducing new APIs.
Specifically, the strict noCache TTL value should simply be a default, with
value -1 = OFF.
Then, introduce a protected method getStrictNoCacheTtl() { return DEFAULT; }
Then in the defined APIs (addResponse, getResponse, etc.):
int noCacheTtl = getStrictNoCacheTtl();
if (response.isStrictNoCache() && noCacheTtl > 0) {
// Strip response and cache object.
}
This should be pretty close to your current code, doesn't add new APIs, and
maintains current behavior while easily allowing scrubbed noCache caching.
http://codereview.appspot.com/4047043/diff/22003/java/gadgets/src/main/java/o...
File
java/gadgets/src/main/java/org/apache/shindig/gadgets/http/AbstractHttpCache.java
(right):
http://codereview.appspot.com/4047043/diff/22003/java/gadgets/src/main/java/o...
java/gadgets/src/main/java/org/apache/shindig/gadgets/http/AbstractHttpCache.java:90:
responseBuilder= new HttpResponseBuilder(response);
nit: space after responseBuilder before =
Sign in to reply to this message.
LGTM. Some minor comments below. http://codereview.appspot.com/4047043/diff/63002/java/gadgets/src/main/java/o... File java/gadgets/src/main/java/org/apache/shindig/gadgets/http/AbstractHttpCache.java (right): http://codereview.appspot.com/4047043/diff/63002/java/gadgets/src/main/java/o... java/gadgets/src/main/java/org/apache/shindig/gadgets/http/AbstractHttpCache.java:47: if (isCacheable(request)) { Extra spacing? http://codereview.appspot.com/4047043/diff/82002/java/gadgets/src/main/java/o... java/gadgets/src/main/java/org/apache/shindig/gadgets/http/AbstractHttpCache.java:47: if (isCacheable(request)) { Extra spacing at end of line? http://codereview.appspot.com/4047043/diff/82002/java/gadgets/src/main/java/o... File java/gadgets/src/main/java/org/apache/shindig/gadgets/http/HttpResponseBuilder.java (right): http://codereview.appspot.com/4047043/diff/82002/java/gadgets/src/main/java/o... java/gadgets/src/main/java/org/apache/shindig/gadgets/http/HttpResponseBuilder.java:37: import java.util.ArrayList; Reorder alphabetically? http://codereview.appspot.com/4047043/diff/82002/java/gadgets/src/test/java/o... File java/gadgets/src/test/java/org/apache/shindig/gadgets/http/AbstractHttpCacheTest.java (right): http://codereview.appspot.com/4047043/diff/82002/java/gadgets/src/test/java/o... java/gadgets/src/test/java/org/apache/shindig/gadgets/http/AbstractHttpCacheTest.java:52: private final ExtendedTtlTestHttpCache extendedTtlCache = new ExtendedTtlTestHttpCache(); Please add a comment above this line indicating that this cache is designed to return 86400 for getStrictNoCacheMaxAge method.
Sign in to reply to this message.
http://codereview.appspot.com/4047043/diff/82002/java/gadgets/src/main/java/o... File java/gadgets/src/main/java/org/apache/shindig/gadgets/http/AbstractHttpCache.java (right): http://codereview.appspot.com/4047043/diff/82002/java/gadgets/src/main/java/o... java/gadgets/src/main/java/org/apache/shindig/gadgets/http/AbstractHttpCache.java:47: if (isCacheable(request)) { On 2011/01/31 14:39:50, anupama.dutta wrote: > Extra spacing at end of line? Done. http://codereview.appspot.com/4047043/diff/82002/java/gadgets/src/main/java/o... File java/gadgets/src/main/java/org/apache/shindig/gadgets/http/HttpResponseBuilder.java (right): http://codereview.appspot.com/4047043/diff/82002/java/gadgets/src/main/java/o... java/gadgets/src/main/java/org/apache/shindig/gadgets/http/HttpResponseBuilder.java:37: import java.util.ArrayList; On 2011/01/31 14:39:50, anupama.dutta wrote: > Reorder alphabetically? Done. http://codereview.appspot.com/4047043/diff/82002/java/gadgets/src/test/java/o... File java/gadgets/src/test/java/org/apache/shindig/gadgets/http/AbstractHttpCacheTest.java (right): http://codereview.appspot.com/4047043/diff/82002/java/gadgets/src/test/java/o... java/gadgets/src/test/java/org/apache/shindig/gadgets/http/AbstractHttpCacheTest.java:52: private final ExtendedTtlTestHttpCache extendedTtlCache = new ExtendedTtlTestHttpCache(); On 2011/01/31 14:39:50, anupama.dutta wrote: > Please add a comment above this line indicating that this cache is designed to > return 86400 for getStrictNoCacheMaxAge method. Done.
Sign in to reply to this message.
Looks good modulo one comment. http://codereview.appspot.com/4047043/diff/86001/java/gadgets/src/main/java/o... File java/gadgets/src/main/java/org/apache/shindig/gadgets/http/HttpResponseBuilder.java (right): http://codereview.appspot.com/4047043/diff/86001/java/gadgets/src/main/java/o... java/gadgets/src/main/java/org/apache/shindig/gadgets/http/HttpResponseBuilder.java:237: public HttpResponseBuilder setCacheControlMaxAge(long expirationTime) { which use case are you trying to accommodate w/ this method vs. simply using setCacheTtl(ttl)?
Sign in to reply to this message.
http://codereview.appspot.com/4047043/diff/86001/java/gadgets/src/main/java/o... File java/gadgets/src/main/java/org/apache/shindig/gadgets/http/HttpResponseBuilder.java (right): http://codereview.appspot.com/4047043/diff/86001/java/gadgets/src/main/java/o... java/gadgets/src/main/java/org/apache/shindig/gadgets/http/HttpResponseBuilder.java:237: public HttpResponseBuilder setCacheControlMaxAge(long expirationTime) { On 2011/01/31 23:22:44, johnfargo wrote: > which use case are you trying to accommodate w/ this method vs. simply using > setCacheTtl(ttl)? setCacheTtl replaces the current cache-control header and removes the pragma header. Since we wanted to keep these headers untouched (so that the resultant response is still strict no-cache), we added this.
Sign in to reply to this message.
Hmm, FMI any examples of such headers you'd still want to keep in this case? I'm curious if I'm not thinking of any downstream effects that would be lost in creating a "simple" nocache response. On Mon, Jan 31, 2011 at 6:01 PM, <nikhilmadan23@gmail.com> wrote: > > > http://codereview.appspot.com/4047043/diff/86001/java/gadgets/src/main/java/o... > File > > java/gadgets/src/main/java/org/apache/shindig/gadgets/http/HttpResponseBuilder.java > (right): > > > http://codereview.appspot.com/4047043/diff/86001/java/gadgets/src/main/java/o... > > java/gadgets/src/main/java/org/apache/shindig/gadgets/http/HttpResponseBuilder.java:237: > public HttpResponseBuilder setCacheControlMaxAge(long expirationTime) { > On 2011/01/31 23:22:44, johnfargo wrote: > >> which use case are you trying to accommodate w/ this method vs. simply >> > using > >> setCacheTtl(ttl)? >> > > setCacheTtl replaces the current cache-control header and removes the > pragma header. Since we wanted to keep these headers untouched (so that > the resultant response is still strict no-cache), we added this. > > > http://codereview.appspot.com/4047043/ >
Sign in to reply to this message.
We want to retain some header which indicates that the response is strict no-cache, and preferably we would like to retain the original header rather than distort it. This will be useful for both logging and debugging purposes. Also, if we use setCacheTtl we will get a public cache-control header. Are you suggesting that we have a setPrivateCacheTtl which will set the Cache-Control header to private and give it the specified max-age? Thanks Nikhil On Tue, Feb 1, 2011 at 9:40 AM, John Hjelmstad <johnfargo@gmail.com> wrote: > Hmm, FMI any examples of such headers you'd still want to keep in this case? > I'm curious if I'm not thinking of any downstream effects that would be lost > in creating a "simple" nocache response. > > On Mon, Jan 31, 2011 at 6:01 PM, <nikhilmadan23@gmail.com> wrote: >> >> >> http://codereview.appspot.com/4047043/diff/86001/java/gadgets/src/main/java/o... >> File >> >> java/gadgets/src/main/java/org/apache/shindig/gadgets/http/HttpResponseBuilder.java >> (right): >> >> >> http://codereview.appspot.com/4047043/diff/86001/java/gadgets/src/main/java/o... >> >> java/gadgets/src/main/java/org/apache/shindig/gadgets/http/HttpResponseBuilder.java:237: >> public HttpResponseBuilder setCacheControlMaxAge(long expirationTime) { >> On 2011/01/31 23:22:44, johnfargo wrote: >>> >>> which use case are you trying to accommodate w/ this method vs. simply >> >> using >>> >>> setCacheTtl(ttl)? >> >> setCacheTtl replaces the current cache-control header and removes the >> pragma header. Since we wanted to keep these headers untouched (so that >> the resultant response is still strict no-cache), we added this. >> >> http://codereview.appspot.com/4047043/ > >
Sign in to reply to this message.
Mostly just wanting to cut down on code and clarify APIs wherever possible. The confusion I guess here stems from the fact that you're AFAIK not planning to use the no-cache results in the HttpCache -- you just want to know they're there so you can avoid proxying/fetching them downstream. So it doesn't really matter in that case whether the cache control header is public or private, at which point we could just augment the setCacheTtl method to have an option to add "no-cache" to a stored result. This said, I don't want to unduly hold up the CL... --j On Mon, Jan 31, 2011 at 8:48 PM, Nikhil <nikhilmadan23@gmail.com> wrote: > We want to retain some header which indicates that the response is > strict no-cache, and preferably we would like to retain the original > header rather than distort it. This will be useful for both logging > and debugging purposes. > Also, if we use setCacheTtl we will get a public cache-control header. > Are you suggesting that we have a setPrivateCacheTtl which will set > the Cache-Control header to private and give it the specified max-age? > > Thanks > Nikhil > > On Tue, Feb 1, 2011 at 9:40 AM, John Hjelmstad <johnfargo@gmail.com> > wrote: > > Hmm, FMI any examples of such headers you'd still want to keep in this > case? > > I'm curious if I'm not thinking of any downstream effects that would be > lost > > in creating a "simple" nocache response. > > > > On Mon, Jan 31, 2011 at 6:01 PM, <nikhilmadan23@gmail.com> wrote: > >> > >> > >> > http://codereview.appspot.com/4047043/diff/86001/java/gadgets/src/main/java/o... > >> File > >> > >> > java/gadgets/src/main/java/org/apache/shindig/gadgets/http/HttpResponseBuilder.java > >> (right): > >> > >> > >> > http://codereview.appspot.com/4047043/diff/86001/java/gadgets/src/main/java/o... > >> > >> > java/gadgets/src/main/java/org/apache/shindig/gadgets/http/HttpResponseBuilder.java:237: > >> public HttpResponseBuilder setCacheControlMaxAge(long expirationTime) { > >> On 2011/01/31 23:22:44, johnfargo wrote: > >>> > >>> which use case are you trying to accommodate w/ this method vs. simply > >> > >> using > >>> > >>> setCacheTtl(ttl)? > >> > >> setCacheTtl replaces the current cache-control header and removes the > >> pragma header. Since we wanted to keep these headers untouched (so that > >> the resultant response is still strict no-cache), we added this. > >> > >> http://codereview.appspot.com/4047043/ > > > > >
Sign in to reply to this message.
Hi Fargo We would feel more comfortable if we could keep the original Cache-Control headers. Setting it to private would mean that this entire piece would become a black box for later testing and/or debugging. Also, augmenting the setCacheTtl with a boolean could result in a number of changes needing to be made throughout shindig since its used at a number of places. Please let me know if you have any other comments regarding the CL. Otherwise, can we please go ahead and commit it. Thanks Nikhil On Wed, Feb 2, 2011 at 4:28 AM, John Hjelmstad <johnfargo@gmail.com> wrote: > Mostly just wanting to cut down on code and clarify APIs wherever possible. > The confusion I guess here stems from the fact that you're AFAIK not > planning to use the no-cache results in the HttpCache -- you just want to > know they're there so you can avoid proxying/fetching them downstream. So it > doesn't really matter in that case whether the cache control header is > public or private, at which point we could just augment the setCacheTtl > method to have an option to add "no-cache" to a stored result. > This said, I don't want to unduly hold up the CL... > --j > > On Mon, Jan 31, 2011 at 8:48 PM, Nikhil <nikhilmadan23@gmail.com> wrote: >> >> We want to retain some header which indicates that the response is >> strict no-cache, and preferably we would like to retain the original >> header rather than distort it. This will be useful for both logging >> and debugging purposes. >> Also, if we use setCacheTtl we will get a public cache-control header. >> Are you suggesting that we have a setPrivateCacheTtl which will set >> the Cache-Control header to private and give it the specified max-age? >> >> Thanks >> Nikhil >> >> On Tue, Feb 1, 2011 at 9:40 AM, John Hjelmstad <johnfargo@gmail.com> >> wrote: >> > Hmm, FMI any examples of such headers you'd still want to keep in this >> > case? >> > I'm curious if I'm not thinking of any downstream effects that would be >> > lost >> > in creating a "simple" nocache response. >> > >> > On Mon, Jan 31, 2011 at 6:01 PM, <nikhilmadan23@gmail.com> wrote: >> >> >> >> >> >> >> >> http://codereview.appspot.com/4047043/diff/86001/java/gadgets/src/main/java/o... >> >> File >> >> >> >> >> >> java/gadgets/src/main/java/org/apache/shindig/gadgets/http/HttpResponseBuilder.java >> >> (right): >> >> >> >> >> >> >> >> http://codereview.appspot.com/4047043/diff/86001/java/gadgets/src/main/java/o... >> >> >> >> >> >> java/gadgets/src/main/java/org/apache/shindig/gadgets/http/HttpResponseBuilder.java:237: >> >> public HttpResponseBuilder setCacheControlMaxAge(long expirationTime) { >> >> On 2011/01/31 23:22:44, johnfargo wrote: >> >>> >> >>> which use case are you trying to accommodate w/ this method vs. simply >> >> >> >> using >> >>> >> >>> setCacheTtl(ttl)? >> >> >> >> setCacheTtl replaces the current cache-control header and removes the >> >> pragma header. Since we wanted to keep these headers untouched (so that >> >> the resultant response is still strict no-cache), we added this. >> >> >> >> http://codereview.appspot.com/4047043/ >> > >> > > >
Sign in to reply to this message.
On Wed, Feb 2, 2011 at 7:04 PM, Nikhil <nikhilmadan23@gmail.com> wrote: > Hi Fargo > > We would feel more comfortable if we could keep the original > Cache-Control headers. Setting it to private would mean that this > entire piece would become a black box for later testing and/or > debugging. > Also, augmenting the setCacheTtl with a boolean could result in a > number of changes needing to be made throughout shindig since its used > at a number of places. > Also, since HttpResponseBuilder (and its setCacheTtl method) would be used extensively even outside of shindig (for instance, inside google code etc.), we think it will be good to leave it undisturbed. > Please let me know if you have any other comments regarding the CL. > Otherwise, can we please go ahead and commit it. > > Thanks > Nikhil > > On Wed, Feb 2, 2011 at 4:28 AM, John Hjelmstad <johnfargo@gmail.com> > wrote: > > Mostly just wanting to cut down on code and clarify APIs wherever > possible. > > The confusion I guess here stems from the fact that you're AFAIK not > > planning to use the no-cache results in the HttpCache -- you just want to > > know they're there so you can avoid proxying/fetching them downstream. So > it > > doesn't really matter in that case whether the cache control header is > > public or private, at which point we could just augment the setCacheTtl > > method to have an option to add "no-cache" to a stored result. > > This said, I don't want to unduly hold up the CL... > > --j > > > > On Mon, Jan 31, 2011 at 8:48 PM, Nikhil <nikhilmadan23@gmail.com> wrote: > >> > >> We want to retain some header which indicates that the response is > >> strict no-cache, and preferably we would like to retain the original > >> header rather than distort it. This will be useful for both logging > >> and debugging purposes. > >> Also, if we use setCacheTtl we will get a public cache-control header. > >> Are you suggesting that we have a setPrivateCacheTtl which will set > >> the Cache-Control header to private and give it the specified max-age? > >> > >> Thanks > >> Nikhil > >> > >> On Tue, Feb 1, 2011 at 9:40 AM, John Hjelmstad <johnfargo@gmail.com> > >> wrote: > >> > Hmm, FMI any examples of such headers you'd still want to keep in this > >> > case? > >> > I'm curious if I'm not thinking of any downstream effects that would > be > >> > lost > >> > in creating a "simple" nocache response. > >> > > >> > On Mon, Jan 31, 2011 at 6:01 PM, <nikhilmadan23@gmail.com> wrote: > >> >> > >> >> > >> >> > >> >> > http://codereview.appspot.com/4047043/diff/86001/java/gadgets/src/main/java/o... > >> >> File > >> >> > >> >> > >> >> > java/gadgets/src/main/java/org/apache/shindig/gadgets/http/HttpResponseBuilder.java > >> >> (right): > >> >> > >> >> > >> >> > >> >> > http://codereview.appspot.com/4047043/diff/86001/java/gadgets/src/main/java/o... > >> >> > >> >> > >> >> > java/gadgets/src/main/java/org/apache/shindig/gadgets/http/HttpResponseBuilder.java:237: > >> >> public HttpResponseBuilder setCacheControlMaxAge(long expirationTime) > { > >> >> On 2011/01/31 23:22:44, johnfargo wrote: > >> >>> > >> >>> which use case are you trying to accommodate w/ this method vs. > simply > >> >> > >> >> using > >> >>> > >> >>> setCacheTtl(ttl)? > >> >> > >> >> setCacheTtl replaces the current cache-control header and removes the > >> >> pragma header. Since we wanted to keep these headers untouched (so > that > >> >> the resultant response is still strict no-cache), we added this. > >> >> > >> >> http://codereview.appspot.com/4047043/ > >> > > >> > > > > > > -- Anupama
Sign in to reply to this message.
LGTM OK, that's rationale enough for me. On Wed, Feb 2, 2011 at 6:10 AM, Anupama Dutta <anupama.dutta@gmail.com>wrote: > On Wed, Feb 2, 2011 at 7:04 PM, Nikhil <nikhilmadan23@gmail.com> wrote: > >> Hi Fargo >> >> We would feel more comfortable if we could keep the original >> Cache-Control headers. Setting it to private would mean that this >> entire piece would become a black box for later testing and/or >> debugging. >> Also, augmenting the setCacheTtl with a boolean could result in a >> number of changes needing to be made throughout shindig since its used >> at a number of places. >> > > Also, since HttpResponseBuilder (and its setCacheTtl method) would be used > extensively even outside of shindig (for instance, inside google code etc.), > we think it will be good to leave it undisturbed. > > >> Please let me know if you have any other comments regarding the CL. >> Otherwise, can we please go ahead and commit it. >> >> Thanks >> Nikhil >> >> On Wed, Feb 2, 2011 at 4:28 AM, John Hjelmstad <johnfargo@gmail.com> >> wrote: >> > Mostly just wanting to cut down on code and clarify APIs wherever >> possible. >> > The confusion I guess here stems from the fact that you're AFAIK not >> > planning to use the no-cache results in the HttpCache -- you just want >> to >> > know they're there so you can avoid proxying/fetching them downstream. >> So it >> > doesn't really matter in that case whether the cache control header is >> > public or private, at which point we could just augment the setCacheTtl >> > method to have an option to add "no-cache" to a stored result. >> > This said, I don't want to unduly hold up the CL... >> > --j >> > >> > On Mon, Jan 31, 2011 at 8:48 PM, Nikhil <nikhilmadan23@gmail.com> >> wrote: >> >> >> >> We want to retain some header which indicates that the response is >> >> strict no-cache, and preferably we would like to retain the original >> >> header rather than distort it. This will be useful for both logging >> >> and debugging purposes. >> >> Also, if we use setCacheTtl we will get a public cache-control header. >> >> Are you suggesting that we have a setPrivateCacheTtl which will set >> >> the Cache-Control header to private and give it the specified max-age? >> >> >> >> Thanks >> >> Nikhil >> >> >> >> On Tue, Feb 1, 2011 at 9:40 AM, John Hjelmstad <johnfargo@gmail.com> >> >> wrote: >> >> > Hmm, FMI any examples of such headers you'd still want to keep in >> this >> >> > case? >> >> > I'm curious if I'm not thinking of any downstream effects that would >> be >> >> > lost >> >> > in creating a "simple" nocache response. >> >> > >> >> > On Mon, Jan 31, 2011 at 6:01 PM, <nikhilmadan23@gmail.com> wrote: >> >> >> >> >> >> >> >> >> >> >> >> >> http://codereview.appspot.com/4047043/diff/86001/java/gadgets/src/main/java/o... >> >> >> File >> >> >> >> >> >> >> >> >> >> java/gadgets/src/main/java/org/apache/shindig/gadgets/http/HttpResponseBuilder.java >> >> >> (right): >> >> >> >> >> >> >> >> >> >> >> >> >> http://codereview.appspot.com/4047043/diff/86001/java/gadgets/src/main/java/o... >> >> >> >> >> >> >> >> >> >> java/gadgets/src/main/java/org/apache/shindig/gadgets/http/HttpResponseBuilder.java:237: >> >> >> public HttpResponseBuilder setCacheControlMaxAge(long >> expirationTime) { >> >> >> On 2011/01/31 23:22:44, johnfargo wrote: >> >> >>> >> >> >>> which use case are you trying to accommodate w/ this method vs. >> simply >> >> >> >> >> >> using >> >> >>> >> >> >>> setCacheTtl(ttl)? >> >> >> >> >> >> setCacheTtl replaces the current cache-control header and removes >> the >> >> >> pragma header. Since we wanted to keep these headers untouched (so >> that >> >> >> the resultant response is still strict no-cache), we added this. >> >> >> >> >> >> http://codereview.appspot.com/4047043/ >> >> > >> >> > >> > >> > >> > > > > -- > Anupama >
Sign in to reply to this message.
Hi all I've changed a couple of things to this CL. I've removed the getStrictNoCacheMaxAge(HttpRequest) method and instead added a new property called 'shindig.cache.http.strict-no-cache-resource.max-age' in java/common/conf/shindig.properties. This is because we will need this at other places as well, for example the getCacheExpiration() method in HttpResponse.java, since it returns -1 for any strict no-cache response, and we would like to keep track of when strict no-cache responses expire. I will address this change in another CL. My apologies for making this change so late into the review. Thanks Nikhil On Wed, Feb 2, 2011 at 11:23 PM, John Hjelmstad <johnfargo@gmail.com> wrote: > LGTM > OK, that's rationale enough for me. > > On Wed, Feb 2, 2011 at 6:10 AM, Anupama Dutta <anupama.dutta@gmail.com> > wrote: >> >> On Wed, Feb 2, 2011 at 7:04 PM, Nikhil <nikhilmadan23@gmail.com> wrote: >>> >>> Hi Fargo >>> >>> We would feel more comfortable if we could keep the original >>> Cache-Control headers. Setting it to private would mean that this >>> entire piece would become a black box for later testing and/or >>> debugging. >>> Also, augmenting the setCacheTtl with a boolean could result in a >>> number of changes needing to be made throughout shindig since its used >>> at a number of places. >> >> Also, since HttpResponseBuilder (and its setCacheTtl method) would be used >> extensively even outside of shindig (for instance, inside google code etc.), >> we think it will be good to leave it undisturbed. >>> >>> Please let me know if you have any other comments regarding the CL. >>> Otherwise, can we please go ahead and commit it. >>> >>> Thanks >>> Nikhil >>> >>> On Wed, Feb 2, 2011 at 4:28 AM, John Hjelmstad <johnfargo@gmail.com> >>> wrote: >>> > Mostly just wanting to cut down on code and clarify APIs wherever >>> > possible. >>> > The confusion I guess here stems from the fact that you're AFAIK not >>> > planning to use the no-cache results in the HttpCache -- you just want >>> > to >>> > know they're there so you can avoid proxying/fetching them downstream. >>> > So it >>> > doesn't really matter in that case whether the cache control header is >>> > public or private, at which point we could just augment the setCacheTtl >>> > method to have an option to add "no-cache" to a stored result. >>> > This said, I don't want to unduly hold up the CL... >>> > --j >>> > >>> > On Mon, Jan 31, 2011 at 8:48 PM, Nikhil <nikhilmadan23@gmail.com> >>> > wrote: >>> >> >>> >> We want to retain some header which indicates that the response is >>> >> strict no-cache, and preferably we would like to retain the original >>> >> header rather than distort it. This will be useful for both logging >>> >> and debugging purposes. >>> >> Also, if we use setCacheTtl we will get a public cache-control header. >>> >> Are you suggesting that we have a setPrivateCacheTtl which will set >>> >> the Cache-Control header to private and give it the specified max-age? >>> >> >>> >> Thanks >>> >> Nikhil >>> >> >>> >> On Tue, Feb 1, 2011 at 9:40 AM, John Hjelmstad <johnfargo@gmail.com> >>> >> wrote: >>> >> > Hmm, FMI any examples of such headers you'd still want to keep in >>> >> > this >>> >> > case? >>> >> > I'm curious if I'm not thinking of any downstream effects that would >>> >> > be >>> >> > lost >>> >> > in creating a "simple" nocache response. >>> >> > >>> >> > On Mon, Jan 31, 2011 at 6:01 PM, <nikhilmadan23@gmail.com> wrote: >>> >> >> >>> >> >> >>> >> >> >>> >> >> >>> >> >> http://codereview.appspot.com/4047043/diff/86001/java/gadgets/src/main/java/o... >>> >> >> File >>> >> >> >>> >> >> >>> >> >> >>> >> >> java/gadgets/src/main/java/org/apache/shindig/gadgets/http/HttpResponseBuilder.java >>> >> >> (right): >>> >> >> >>> >> >> >>> >> >> >>> >> >> >>> >> >> http://codereview.appspot.com/4047043/diff/86001/java/gadgets/src/main/java/o... >>> >> >> >>> >> >> >>> >> >> >>> >> >> java/gadgets/src/main/java/org/apache/shindig/gadgets/http/HttpResponseBuilder.java:237: >>> >> >> public HttpResponseBuilder setCacheControlMaxAge(long >>> >> >> expirationTime) { >>> >> >> On 2011/01/31 23:22:44, johnfargo wrote: >>> >> >>> >>> >> >>> which use case are you trying to accommodate w/ this method vs. >>> >> >>> simply >>> >> >> >>> >> >> using >>> >> >>> >>> >> >>> setCacheTtl(ttl)? >>> >> >> >>> >> >> setCacheTtl replaces the current cache-control header and removes >>> >> >> the >>> >> >> pragma header. Since we wanted to keep these headers untouched (so >>> >> >> that >>> >> >> the resultant response is still strict no-cache), we added this. >>> >> >> >>> >> >> http://codereview.appspot.com/4047043/ >>> >> > >>> >> > >>> > >>> > >> >> >> >> -- >> Anupama > >
Sign in to reply to this message.
LGTM. Minor comment below. http://codereview.appspot.com/4047043/diff/84004/java/gadgets/src/main/java/o... File java/gadgets/src/main/java/org/apache/shindig/gadgets/http/AbstractHttpCache.java (right): http://codereview.appspot.com/4047043/diff/84004/java/gadgets/src/main/java/o... java/gadgets/src/main/java/org/apache/shindig/gadgets/http/AbstractHttpCache.java:71: if (isCacheable(request, response, storeStrictNoCacheResources)) { Remove extra space after last comma.
Sign in to reply to this message.
http://codereview.appspot.com/4047043/diff/84004/java/gadgets/src/main/java/o... File java/gadgets/src/main/java/org/apache/shindig/gadgets/http/AbstractHttpCache.java (right): http://codereview.appspot.com/4047043/diff/84004/java/gadgets/src/main/java/o... java/gadgets/src/main/java/org/apache/shindig/gadgets/http/AbstractHttpCache.java:71: if (isCacheable(request, response, storeStrictNoCacheResources)) { On 2011/02/03 15:15:59, anupama.dutta wrote: > Remove extra space after last comma. Done.
Sign in to reply to this message.
http://codereview.appspot.com/4047043/diff/90002/java/gadgets/src/main/java/o... File java/gadgets/src/main/java/org/apache/shindig/gadgets/http/AbstractHttpCache.java (right): http://codereview.appspot.com/4047043/diff/90002/java/gadgets/src/main/java/o... java/gadgets/src/main/java/org/apache/shindig/gadgets/http/AbstractHttpCache.java:37: * of building your own keys from scratch. Might want to mention in the class level comment since this is a big change in the sense that it breaks previous assumption that cache control private resources will not be cached. http://codereview.appspot.com/4047043/diff/90002/java/gadgets/src/main/java/o... java/gadgets/src/main/java/org/apache/shindig/gadgets/http/AbstractHttpCache.java:49: private long strictNoCacheResourceTtl = DEFAULT_STRICT_NO_CACHE_RESOURCE_TTL; this is in seconds? please add a comment noting the units, or say strictNoCacheResourceTtlSec http://codereview.appspot.com/4047043/diff/90002/java/gadgets/src/main/java/o... java/gadgets/src/main/java/org/apache/shindig/gadgets/http/AbstractHttpCache.java:61: (!cached.isStrictNoCache() || strictNoCacheResourceTtl > 0)) { shouldn't you also modify DefaultRequestPipeline to ignore such dummy cache control resources ? Otherwise if some1 sets shindig.cache.http.strict-no-cache-resource.max-age to non-zero, they'l start serving these dummy response. http://codereview.appspot.com/4047043/diff/90002/java/gadgets/src/main/java/o... java/gadgets/src/main/java/org/apache/shindig/gadgets/http/AbstractHttpCache.java:73: (storeStrictNoCacheResources && !response.isStrictNoCache())) { if (storeStrictNoCacheResources && response.isStrictNoCache()) { responseBuilder = buildStrictNoCacheHttpResponse(request, response); } else { responseBuilder = new HttpResponseBuilder(response); } might be more readable http://codereview.appspot.com/4047043/diff/90002/java/gadgets/src/main/java/o... File java/gadgets/src/main/java/org/apache/shindig/gadgets/http/HttpResponseBuilder.java (right): http://codereview.appspot.com/4047043/diff/90002/java/gadgets/src/main/java/o... java/gadgets/src/main/java/org/apache/shindig/gadgets/http/HttpResponseBuilder.java:239: ArrayList<String> directives = new ArrayList<String>(); List<String> directives = Lists.newArrayList(); http://codereview.appspot.com/4047043/diff/90002/java/gadgets/src/main/java/o... java/gadgets/src/main/java/org/apache/shindig/gadgets/http/HttpResponseBuilder.java:241: directives.addAll(Arrays.asList(StringUtils.split(cacheControl, ','))); for (String directive : StringUtils.split(cacheControl, ',')) { if (!directive.startsWith("max-age") || StringUtils.split(directive, '=').length != 2) { directives.add(directive); } }
Sign in to reply to this message.
http://codereview.appspot.com/4047043/diff/90002/java/gadgets/src/main/java/o... File java/gadgets/src/main/java/org/apache/shindig/gadgets/http/AbstractHttpCache.java (right): http://codereview.appspot.com/4047043/diff/90002/java/gadgets/src/main/java/o... java/gadgets/src/main/java/org/apache/shindig/gadgets/http/AbstractHttpCache.java:37: * of building your own keys from scratch. On 2011/02/07 18:58:56, gagan.goku wrote: > Might want to mention in the class level comment since this is a big change in > the sense that it breaks previous assumption that cache control private > resources will not be cached. Done. http://codereview.appspot.com/4047043/diff/90002/java/gadgets/src/main/java/o... java/gadgets/src/main/java/org/apache/shindig/gadgets/http/AbstractHttpCache.java:49: private long strictNoCacheResourceTtl = DEFAULT_STRICT_NO_CACHE_RESOURCE_TTL; On 2011/02/07 18:58:56, gagan.goku wrote: > this is in seconds? please add a comment noting the units, > or say strictNoCacheResourceTtlSec Done. http://codereview.appspot.com/4047043/diff/90002/java/gadgets/src/main/java/o... java/gadgets/src/main/java/org/apache/shindig/gadgets/http/AbstractHttpCache.java:61: (!cached.isStrictNoCache() || strictNoCacheResourceTtl > 0)) { On 2011/02/07 18:58:56, gagan.goku wrote: > shouldn't you also modify DefaultRequestPipeline to ignore such dummy cache > control resources ? > Otherwise if some1 sets shindig.cache.http.strict-no-cache-resource.max-age to > non-zero, they'l start serving these dummy response. Good point. Done. http://codereview.appspot.com/4047043/diff/90002/java/gadgets/src/main/java/o... java/gadgets/src/main/java/org/apache/shindig/gadgets/http/AbstractHttpCache.java:73: (storeStrictNoCacheResources && !response.isStrictNoCache())) { On 2011/02/07 18:58:56, gagan.goku wrote: > if (storeStrictNoCacheResources && response.isStrictNoCache()) { > responseBuilder = buildStrictNoCacheHttpResponse(request, response); > } else { > responseBuilder = new HttpResponseBuilder(response); > } > might be more readable Done. http://codereview.appspot.com/4047043/diff/90002/java/gadgets/src/main/java/o... File java/gadgets/src/main/java/org/apache/shindig/gadgets/http/HttpResponseBuilder.java (right): http://codereview.appspot.com/4047043/diff/90002/java/gadgets/src/main/java/o... java/gadgets/src/main/java/org/apache/shindig/gadgets/http/HttpResponseBuilder.java:239: ArrayList<String> directives = new ArrayList<String>(); On 2011/02/07 18:58:56, gagan.goku wrote: > List<String> directives = Lists.newArrayList(); Done. http://codereview.appspot.com/4047043/diff/90002/java/gadgets/src/main/java/o... java/gadgets/src/main/java/org/apache/shindig/gadgets/http/HttpResponseBuilder.java:241: directives.addAll(Arrays.asList(StringUtils.split(cacheControl, ','))); On 2011/02/07 18:58:56, gagan.goku wrote: > for (String directive : StringUtils.split(cacheControl, ',')) { > if (!directive.startsWith("max-age") || StringUtils.split(directive, > '=').length != 2) { > directives.add(directive); > } > } Done.
Sign in to reply to this message.
lgtm http://codereview.appspot.com/4047043/diff/94001/java/gadgets/src/main/java/o... File java/gadgets/src/main/java/org/apache/shindig/gadgets/http/DefaultRequestPipeline.java (right): http://codereview.appspot.com/4047043/diff/94001/java/gadgets/src/main/java/o... java/gadgets/src/main/java/org/apache/shindig/gadgets/http/DefaultRequestPipeline.java:84: if (cachedResponse != null && !cachedResponse.isStrictNoCache()) { just mention here as a comment that srict no-cache resources are dummy resources and should not be used.
Sign in to reply to this message.
http://codereview.appspot.com/4047043/diff/94001/java/gadgets/src/main/java/o... File java/gadgets/src/main/java/org/apache/shindig/gadgets/http/DefaultRequestPipeline.java (right): http://codereview.appspot.com/4047043/diff/94001/java/gadgets/src/main/java/o... java/gadgets/src/main/java/org/apache/shindig/gadgets/http/DefaultRequestPipeline.java:84: if (cachedResponse != null && !cachedResponse.isStrictNoCache()) { On 2011/02/08 09:05:27, gagan.goku wrote: > just mention here as a comment that srict no-cache resources are dummy resources > and should not be used. Done.
Sign in to reply to this message.
|
