Issue 4260053: Fixing CacheEnforcementVisitor broken in 1076489.
|
Created:
15 years ago by atulvasu Modified:
15 years ago Base URL:
http://svn.apache.org/repos/asf/shindig/trunk/ Visibility:
Public. |
Description
1076489 changed a static variable to non-static. Unfortunately tests were written poorly and was still passing. Fixed both the test, and ensured that we are not changing static variables in test.
Patch Set 1 #Patch Set 2 : Fixing CacheEnforcementVisitor to handle private/nocache/nostore conditions correctly. #Patch Set 3 : Fixing HttpResponse to have static strictNoCacheResourceTtl value. #Patch Set 4 : Fixing strict no cache expiration issues #Patch Set 5 : Fixing strict no cache expiration issues #
Total comments: 6
Patch Set 6 : Fixing strict no cache expiration issues #Patch Set 7 : Fixing strict no cache expiration issues #Patch Set 8 : Fixing strict no cache expiration issues #
Total comments: 4
Patch Set 9 : Fixing strict no cache expiration issues #Patch Set 10 : Fixing strict no cache expiration issues #Patch Set 11 : Fixing strict no cache expiration issues #Patch Set 12 : Fixing strict no cache expiration issues #Patch Set 13 : Fixing strict no cache expiration issues #Patch Set 14 : Fixing strict no cache expiration issues #
Total comments: 13
Patch Set 15 : Fixing strict no cache expiration issues #
Total comments: 12
Patch Set 16 : Fixing strict no cache expiration issues #
Total comments: 2
Patch Set 17 : Fixing CacheEnforcementVisitor to handle private/nocache/nostore conditions correctly #Patch Set 18 : Fixing CacheEnforcementVisitor to handle private/nocache/nostore conditions correctly #Patch Set 19 : Fixing CacheEnforcementVisitor broken in 1076489 #Patch Set 20 : Fixing CacheEnforcementVisitor broken in 1076489 #
Total comments: 19
Patch Set 21 : Fixing CacheEnforcementVisitor broken in 1076489 #Patch Set 22 : Fixing CacheEnforcementVisitor broken in 1076489 #Patch Set 23 : Fixing CacheEnforcementVisitor broken in 1076489 #Patch Set 24 : Fixing CacheEnforcementVisitor broken in 1076489 #Patch Set 25 : Fixing CacheEnforcementVisitor broken in 1076489 #Patch Set 26 : Fixing CacheEnforcementVisitor broken in 1076489 #
Total comments: 22
Patch Set 27 : Fixing CacheEnforcementVisitor broken in 1076489 #Patch Set 28 : Fixing CacheEnforcementVisitor broken in 1076489 #Patch Set 29 : Fixing CacheEnforcementVisitor broken in 1076489 #
Total comments: 12
Patch Set 30 : Fixing CacheEnforcementVisitor broken in 1076489 #
Total comments: 4
Patch Set 31 : Fixing CacheEnforcementVisitor broken in 1076489 #MessagesTotal messages: 51
Lgtm. Please ask Ziv / Jacobo why this change was reverted.
Sign in to reply to this message.
On 2011/03/07 11:02:24, gagan.goku wrote: > Lgtm. > Please ask Ziv / Jacobo why this change was reverted. I am somehow not able to follow this change - the patch description says "fixing CacheEnforcementVisitor" and the patch does not seem to touch CacheEnforcementVisitor. Atul, could you clarify? Also, Gagan, what was the change that was reverted? Thanks, Anupama.
Sign in to reply to this message.
Two things are broken: 1) Should not change the value of static objects from test because they conflict each other. 2) Should not use singlethreadexecutor because they mask errors. What happened (1) & (2) simultaneously tests were passing, (1) was fixed (??) by changing from static to non-static, but tests started failing because of (2), but since it is a different thread nobody knew. Now the actual fix is much harder. I am still looking about how to do it cleanly. On Mon, Mar 7, 2011 at 5:50 PM, <anupama.dutta@gmail.com> wrote: > On 2011/03/07 11:02:24, gagan.goku wrote: > >> Lgtm. >> Please ask Ziv / Jacobo why this change was reverted. >> > > I am somehow not able to follow this change - the patch description says > "fixing CacheEnforcementVisitor" and the patch does not seem to touch > CacheEnforcementVisitor. Atul, could you clarify? > > Also, Gagan, what was the change that was reverted? > > Thanks, > Anupama. > > > http://codereview.appspot.com/4260053/ >
Sign in to reply to this message.
Hi Anupama Jacobo reverted the strictNoCacheResourceTtl to non-static because he was seeing some test failures. Look at this cl: http://codereview.appspot.com/4240065/ Atul started with fixing CacheEnforcementVisitor for something, and then we ended up adding back the static keyword and thought Nikhil had missed it. Later with Pulkit we realized that they had removed it intentionally because of some test failures. So the files touched in the codereview have changed but the cl description has not. Thanks Gagan On Mon, Mar 7, 2011 at 5:50 PM, <anupama.dutta@gmail.com> wrote: > On 2011/03/07 11:02:24, gagan.goku wrote: > >> Lgtm. >> Please ask Ziv / Jacobo why this change was reverted. >> > > I am somehow not able to follow this change - the patch description says > "fixing CacheEnforcementVisitor" and the patch does not seem to touch > CacheEnforcementVisitor. Atul, could you clarify? > > Also, Gagan, what was the change that was reverted? > > Thanks, > Anupama. > > > http://codereview.appspot.com/4260053/ >
Sign in to reply to this message.
Ok. Got it now. (Pulkit also mentioned this problem (of using static injection in tests) to me in the context of one of his current code reviews today.) Two comments/queries: - Atul, could you update the patch description to indicate exactly what you are doing in this patch now? - And, are the tests fixed and stable with your current changes or are there still changes to be tried out as part of this change? Thanks, Anupama. On Mon, Mar 7, 2011 at 6:28 PM, Gagandeep singh <gagan.goku@gmail.com>wrote: > Hi Anupama > > Jacobo reverted the strictNoCacheResourceTtl to non-static because he was > seeing some test failures. Look at this cl: > http://codereview.appspot.com/4240065/ > Atul started with fixing CacheEnforcementVisitor for something, and then > we ended up adding back the static keyword and thought Nikhil had missed it. > Later with Pulkit we realized that they had removed it intentionally because > of some test failures. > So the files touched in the codereview have changed but the cl description > has not. > > Thanks > Gagan > > > On Mon, Mar 7, 2011 at 5:50 PM, <anupama.dutta@gmail.com> wrote: > >> On 2011/03/07 11:02:24, gagan.goku wrote: >> >>> Lgtm. >>> Please ask Ziv / Jacobo why this change was reverted. >>> >> >> I am somehow not able to follow this change - the patch description says >> "fixing CacheEnforcementVisitor" and the patch does not seem to touch >> CacheEnforcementVisitor. Atul, could you clarify? >> >> Also, Gagan, what was the change that was reverted? >> >> Thanks, >> Anupama. >> >> >> http://codereview.appspot.com/4260053/ >> >
Sign in to reply to this message.
This patch is wrong, I am not decided what to do. Initially I thought I'll just make it non-static and inject at ResponseBuilder. But some unusual problems with that too, because of the way AbstractHttpCacheTest is written, so moving it to AbstractHttpCache instead, but that causes problems in ServletUtil,... I don't know where to put it exactly... So I'll make a new patch with complete description when done. On Mon, Mar 7, 2011 at 7:26 PM, Anupama Dutta <anupama.dutta@gmail.com>wrote: > Ok. Got it now. (Pulkit also mentioned this problem (of using static > injection in tests) to me in the context of one of his current code reviews > today.) Two comments/queries: > - Atul, could you update the patch description to indicate exactly what you > are doing in this patch now? > - And, are the tests fixed and stable with your current changes or are > there still changes to be tried out as part of this change? > > Thanks, > Anupama. > > On Mon, Mar 7, 2011 at 6:28 PM, Gagandeep singh <gagan.goku@gmail.com>wrote: > >> Hi Anupama >> >> Jacobo reverted the strictNoCacheResourceTtl to non-static because he was >> seeing some test failures. Look at this cl: >> http://codereview.appspot.com/4240065/ >> Atul started with fixing CacheEnforcementVisitor for something, and then >> we ended up adding back the static keyword and thought Nikhil had missed it. >> Later with Pulkit we realized that they had removed it intentionally because >> of some test failures. >> So the files touched in the codereview have changed but the cl description >> has not. >> >> Thanks >> Gagan >> >> >> On Mon, Mar 7, 2011 at 5:50 PM, <anupama.dutta@gmail.com> wrote: >> >>> On 2011/03/07 11:02:24, gagan.goku wrote: >>> >>>> Lgtm. >>>> Please ask Ziv / Jacobo why this change was reverted. >>>> >>> >>> I am somehow not able to follow this change - the patch description says >>> "fixing CacheEnforcementVisitor" and the patch does not seem to touch >>> CacheEnforcementVisitor. Atul, could you clarify? >>> >>> Also, Gagan, what was the change that was reverted? >>> >>> Thanks, >>> Anupama. >>> >>> >>> http://codereview.appspot.com/4260053/ >>> >> >
Sign in to reply to this message.
In other words ignore this Issue for now. On Mon, Mar 7, 2011 at 7:29 PM, Atul S Vasu <atulvasu@google.com> wrote: > This patch is wrong, I am not decided what to do. Initially I thought I'll > just make it non-static and inject at ResponseBuilder. > But some unusual problems with that too, because of the way > AbstractHttpCacheTest is written, so moving it to AbstractHttpCache > instead, but that causes problems in ServletUtil,... I don't know where to > put it exactly... So I'll make a new patch with complete > description when done. > > > On Mon, Mar 7, 2011 at 7:26 PM, Anupama Dutta <anupama.dutta@gmail.com>wrote: > >> Ok. Got it now. (Pulkit also mentioned this problem (of using static >> injection in tests) to me in the context of one of his current code reviews >> today.) Two comments/queries: >> - Atul, could you update the patch description to indicate exactly what >> you are doing in this patch now? >> - And, are the tests fixed and stable with your current changes or are >> there still changes to be tried out as part of this change? >> >> Thanks, >> Anupama. >> >> On Mon, Mar 7, 2011 at 6:28 PM, Gagandeep singh <gagan.goku@gmail.com>wrote: >> >>> Hi Anupama >>> >>> Jacobo reverted the strictNoCacheResourceTtl to non-static because he >>> was seeing some test failures. Look at this cl: >>> http://codereview.appspot.com/4240065/ >>> Atul started with fixing CacheEnforcementVisitor for something, and then >>> we ended up adding back the static keyword and thought Nikhil had missed it. >>> Later with Pulkit we realized that they had removed it intentionally because >>> of some test failures. >>> So the files touched in the codereview have changed but the cl >>> description has not. >>> >>> Thanks >>> Gagan >>> >>> >>> On Mon, Mar 7, 2011 at 5:50 PM, <anupama.dutta@gmail.com> wrote: >>> >>>> On 2011/03/07 11:02:24, gagan.goku wrote: >>>> >>>>> Lgtm. >>>>> Please ask Ziv / Jacobo why this change was reverted. >>>>> >>>> >>>> I am somehow not able to follow this change - the patch description says >>>> "fixing CacheEnforcementVisitor" and the patch does not seem to touch >>>> CacheEnforcementVisitor. Atul, could you clarify? >>>> >>>> Also, Gagan, what was the change that was reverted? >>>> >>>> Thanks, >>>> Anupama. >>>> >>>> >>>> http://codereview.appspot.com/4260053/ >>>> >>> >> >
Sign in to reply to this message.
Nice summary. On Mar 7, 2011 7:29 PM, "Atul S Vasu" <atulvasu@google.com> wrote: > In other words ignore this Issue for now. > > On Mon, Mar 7, 2011 at 7:29 PM, Atul S Vasu <atulvasu@google.com> wrote: > >> This patch is wrong, I am not decided what to do. Initially I thought I'll >> just make it non-static and inject at ResponseBuilder. >> But some unusual problems with that too, because of the way >> AbstractHttpCacheTest is written, so moving it to AbstractHttpCache >> instead, but that causes problems in ServletUtil,... I don't know where to >> put it exactly... So I'll make a new patch with complete >> description when done. >> >> >> On Mon, Mar 7, 2011 at 7:26 PM, Anupama Dutta <anupama.dutta@gmail.com >wrote: >> >>> Ok. Got it now. (Pulkit also mentioned this problem (of using static >>> injection in tests) to me in the context of one of his current code reviews >>> today.) Two comments/queries: >>> - Atul, could you update the patch description to indicate exactly what >>> you are doing in this patch now? >>> - And, are the tests fixed and stable with your current changes or are >>> there still changes to be tried out as part of this change? >>> >>> Thanks, >>> Anupama. >>> >>> On Mon, Mar 7, 2011 at 6:28 PM, Gagandeep singh <gagan.goku@gmail.com >wrote: >>> >>>> Hi Anupama >>>> >>>> Jacobo reverted the strictNoCacheResourceTtl to non-static because he >>>> was seeing some test failures. Look at this cl: >>>> http://codereview.appspot.com/4240065/ >>>> Atul started with fixing CacheEnforcementVisitor for something, and then >>>> we ended up adding back the static keyword and thought Nikhil had missed it. >>>> Later with Pulkit we realized that they had removed it intentionally because >>>> of some test failures. >>>> So the files touched in the codereview have changed but the cl >>>> description has not. >>>> >>>> Thanks >>>> Gagan >>>> >>>> >>>> On Mon, Mar 7, 2011 at 5:50 PM, <anupama.dutta@gmail.com> wrote: >>>> >>>>> On 2011/03/07 11:02:24, gagan.goku wrote: >>>>> >>>>>> Lgtm. >>>>>> Please ask Ziv / Jacobo why this change was reverted. >>>>>> >>>>> >>>>> I am somehow not able to follow this change - the patch description says >>>>> "fixing CacheEnforcementVisitor" and the patch does not seem to touch >>>>> CacheEnforcementVisitor. Atul, could you clarify? >>>>> >>>>> Also, Gagan, what was the change that was reverted? >>>>> >>>>> Thanks, >>>>> Anupama. >>>>> >>>>> >>>>> http://codereview.appspot.com/4260053/ >>>>> >>>> >>> >>
Sign in to reply to this message.
Keep in mind that CacheEnforcementVisitor is not in working condition now though. On Mon, Mar 7, 2011 at 9:20 PM, Gagandeep Singh <gagan.goku@gmail.com>wrote: > Nice summary. > On Mar 7, 2011 7:29 PM, "Atul S Vasu" <atulvasu@google.com> wrote: > > In other words ignore this Issue for now. > > > > On Mon, Mar 7, 2011 at 7:29 PM, Atul S Vasu <atulvasu@google.com> wrote: > > > >> This patch is wrong, I am not decided what to do. Initially I thought > I'll > >> just make it non-static and inject at ResponseBuilder. > >> But some unusual problems with that too, because of the way > >> AbstractHttpCacheTest is written, so moving it to AbstractHttpCache > >> instead, but that causes problems in ServletUtil,... I don't know where > to > >> put it exactly... So I'll make a new patch with complete > >> description when done. > >> > >> > >> On Mon, Mar 7, 2011 at 7:26 PM, Anupama Dutta <anupama.dutta@gmail.com > >wrote: > >> > >>> Ok. Got it now. (Pulkit also mentioned this problem (of using static > >>> injection in tests) to me in the context of one of his current code > reviews > >>> today.) Two comments/queries: > >>> - Atul, could you update the patch description to indicate exactly what > >>> you are doing in this patch now? > >>> - And, are the tests fixed and stable with your current changes or are > >>> there still changes to be tried out as part of this change? > >>> > >>> Thanks, > >>> Anupama. > >>> > >>> On Mon, Mar 7, 2011 at 6:28 PM, Gagandeep singh <gagan.goku@gmail.com > >wrote: > >>> > >>>> Hi Anupama > >>>> > >>>> Jacobo reverted the strictNoCacheResourceTtl to non-static because he > >>>> was seeing some test failures. Look at this cl: > >>>> http://codereview.appspot.com/4240065/ > >>>> Atul started with fixing CacheEnforcementVisitor for something, and > then > >>>> we ended up adding back the static keyword and thought Nikhil had > missed it. > >>>> Later with Pulkit we realized that they had removed it intentionally > because > >>>> of some test failures. > >>>> So the files touched in the codereview have changed but the cl > >>>> description has not. > >>>> > >>>> Thanks > >>>> Gagan > >>>> > >>>> > >>>> On Mon, Mar 7, 2011 at 5:50 PM, <anupama.dutta@gmail.com> wrote: > >>>> > >>>>> On 2011/03/07 11:02:24, gagan.goku wrote: > >>>>> > >>>>>> Lgtm. > >>>>>> Please ask Ziv / Jacobo why this change was reverted. > >>>>>> > >>>>> > >>>>> I am somehow not able to follow this change - the patch description > says > >>>>> "fixing CacheEnforcementVisitor" and the patch does not seem to touch > >>>>> CacheEnforcementVisitor. Atul, could you clarify? > >>>>> > >>>>> Also, Gagan, what was the change that was reverted? > >>>>> > >>>>> Thanks, > >>>>> Anupama. > >>>>> > >>>>> > >>>>> http://codereview.appspot.com/4260053/ > >>>>> > >>>> > >>> > >> >
Sign in to reply to this message.
You can start reviewing this CL. On Tue, Mar 8, 2011 at 4:36 PM, <atulvasu@google.com> wrote: > http://codereview.appspot.com/4260053/ >
Sign in to reply to this message.
http://codereview.appspot.com/4260053/diff/14002/java/gadgets/src/main/java/o... File java/gadgets/src/main/java/org/apache/shindig/gadgets/http/AbstractHttpCache.java (right): http://codereview.appspot.com/4260053/diff/14002/java/gadgets/src/main/java/o... java/gadgets/src/main/java/org/apache/shindig/gadgets/http/AbstractHttpCache.java:51: // considered as expired as long as they are haven't expired otherwise. as long as they haven't http://codereview.appspot.com/4260053/diff/14002/java/gadgets/src/main/java/o... File java/gadgets/src/main/java/org/apache/shindig/gadgets/http/HttpResponse.java (right): http://codereview.appspot.com/4260053/diff/14002/java/gadgets/src/main/java/o... java/gadgets/src/main/java/org/apache/shindig/gadgets/http/HttpResponse.java:130: // and expires headers. Resources without cache-control is considered public by default. are considered http://codereview.appspot.com/4260053/diff/14002/java/gadgets/src/main/java/o... java/gadgets/src/main/java/org/apache/shindig/gadgets/http/HttpResponse.java:359: public long getInternalCacheDuration() { so this means that private content will always retain the cacheMaxAgeOverride? This could lead to problems if a publisher changes a resource from private to public. Could we instead return cacheMaxAgeOverride - getTimeSource().currentTimeMillis()? Also could you add tests for this file?
Sign in to reply to this message.
http://codereview.appspot.com/4260053/diff/14002/java/gadgets/src/main/java/o... File java/gadgets/src/main/java/org/apache/shindig/gadgets/http/AbstractHttpCache.java (right): http://codereview.appspot.com/4260053/diff/14002/java/gadgets/src/main/java/o... java/gadgets/src/main/java/org/apache/shindig/gadgets/http/AbstractHttpCache.java:51: // considered as expired as long as they are haven't expired otherwise. On 2011/03/08 11:49:21, nikhilmadan23 wrote: > as long as they haven't Done. http://codereview.appspot.com/4260053/diff/14002/java/gadgets/src/main/java/o... File java/gadgets/src/main/java/org/apache/shindig/gadgets/http/HttpResponse.java (right): http://codereview.appspot.com/4260053/diff/14002/java/gadgets/src/main/java/o... java/gadgets/src/main/java/org/apache/shindig/gadgets/http/HttpResponse.java:130: // and expires headers. Resources without cache-control is considered public by default. On 2011/03/08 11:49:21, nikhilmadan23 wrote: > are considered Done. http://codereview.appspot.com/4260053/diff/14002/java/gadgets/src/main/java/o... java/gadgets/src/main/java/org/apache/shindig/gadgets/http/HttpResponse.java:359: public long getInternalCacheDuration() { On 2011/03/08 11:49:21, nikhilmadan23 wrote: > so this means that private content will always retain the cacheMaxAgeOverride? > This could lead to problems if a publisher changes a resource from private to > public. Could we instead return cacheMaxAgeOverride - > getTimeSource().currentTimeMillis()? > > Also could you add tests for this file? it will get subtracted automatically in getCacheTtl();
Sign in to reply to this message.
LGTM Could you add tests for HttpResponse?
Sign in to reply to this message.
On 2011/03/08 12:48:48, nikhilmadan23 wrote: > LGTM > > Could you add tests for HttpResponse? Added tests, and requestStaticInjection for AbstractHttpCache from DefaultGuiceModule
Sign in to reply to this message.
lgtm... few minor nits http://codereview.appspot.com/4260053/diff/7002/java/gadgets/src/main/java/or... File java/gadgets/src/main/java/org/apache/shindig/gadgets/http/HttpResponseBuilder.java (right): http://codereview.appspot.com/4260053/diff/7002/java/gadgets/src/main/java/or... java/gadgets/src/main/java/org/apache/shindig/gadgets/http/HttpResponseBuilder.java:25: import com.google.inject.Inject; not needed http://codereview.appspot.com/4260053/diff/7002/java/gadgets/src/test/java/or... File java/gadgets/src/test/java/org/apache/shindig/gadgets/http/HttpResponseTest.java (right): http://codereview.appspot.com/4260053/diff/7002/java/gadgets/src/test/java/or... java/gadgets/src/test/java/org/apache/shindig/gadgets/http/HttpResponseTest.java:20: import org.apache.shindig.common.servlet.HttpUtil; not needed
Sign in to reply to this message.
http://codereview.appspot.com/4260053/diff/7002/java/gadgets/src/main/java/or... File java/gadgets/src/main/java/org/apache/shindig/gadgets/http/HttpResponseBuilder.java (right): http://codereview.appspot.com/4260053/diff/7002/java/gadgets/src/main/java/or... java/gadgets/src/main/java/org/apache/shindig/gadgets/http/HttpResponseBuilder.java:25: import com.google.inject.Inject; On 2011/03/08 13:45:58, nikhilmadan23 wrote: > not needed Done. http://codereview.appspot.com/4260053/diff/7002/java/gadgets/src/test/java/or... File java/gadgets/src/test/java/org/apache/shindig/gadgets/http/HttpResponseTest.java (right): http://codereview.appspot.com/4260053/diff/7002/java/gadgets/src/test/java/or... java/gadgets/src/test/java/org/apache/shindig/gadgets/http/HttpResponseTest.java:20: import org.apache.shindig.common.servlet.HttpUtil; On 2011/03/08 13:45:58, nikhilmadan23 wrote: > not needed Done.
Sign in to reply to this message.
http://codereview.appspot.com/4260053/diff/22001/java/gadgets/src/main/java/o... File java/gadgets/src/main/java/org/apache/shindig/gadgets/http/HttpResponse.java (right): http://codereview.appspot.com/4260053/diff/22001/java/gadgets/src/main/java/o... java/gadgets/src/main/java/org/apache/shindig/gadgets/http/HttpResponse.java:22: import com.google.common.collect.ImmutableList; its okay to keep this wildcard. Run the optimize imports plugin in intellij, it usually does the right thing. http://codereview.appspot.com/4260053/diff/22001/java/gadgets/src/main/java/o... java/gadgets/src/main/java/org/apache/shindig/gadgets/http/HttpResponse.java:367: private long getInternalCacheMaxAge() { might as well get rid of this function, its just adding to confusion http://codereview.appspot.com/4260053/diff/22001/java/gadgets/src/main/java/o... java/gadgets/src/main/java/org/apache/shindig/gadgets/http/HttpResponse.java:375: public long getCacheMaxAgeOverride() { please document in what cases this override is overriding something that is being overridden. http://codereview.appspot.com/4260053/diff/22001/java/gadgets/src/main/java/o... File java/gadgets/src/main/java/org/apache/shindig/gadgets/http/HttpResponseBuilder.java (right): http://codereview.appspot.com/4260053/diff/22001/java/gadgets/src/main/java/o... java/gadgets/src/main/java/org/apache/shindig/gadgets/http/HttpResponseBuilder.java:55: private long cacheMaxAgeOverride = -1; actually this is not really overriding the max age, this is the max-age in case of noCache resources. so maybe rename this variable to strictNoCacheMaxAge everywhere. http://codereview.appspot.com/4260053/diff/22001/java/gadgets/src/test/java/o... File java/gadgets/src/test/java/org/apache/shindig/gadgets/http/AbstractHttpCacheTest.java (right): http://codereview.appspot.com/4260053/diff/22001/java/gadgets/src/test/java/o... java/gadgets/src/test/java/org/apache/shindig/gadgets/http/AbstractHttpCacheTest.java:458: assertEquals("private", builtResponse.getHeader("Cache-Control")); this doesn't seem right, why is max-age removed from the cache-control header? there might be other people / future uses that might want to look at the max-age value by looking at the header and decide whether to refetch it again or process it depending on its max-age. http://codereview.appspot.com/4260053/diff/22001/java/gadgets/src/test/java/o... File java/gadgets/src/test/java/org/apache/shindig/gadgets/http/HttpResponseTest.java (right): http://codereview.appspot.com/4260053/diff/22001/java/gadgets/src/test/java/o... java/gadgets/src/test/java/org/apache/shindig/gadgets/http/HttpResponseTest.java:536: roundToSeconds(new HttpResponseBuilder().setStrictNoCache().setCacheMaxAgeOverride(10000) new HttpResponseBuilder() .setStrictNoCache() .setCacheMaxAgeOverride(10000) .create() .getCacheExpiration() for readability. Nice chaining though :) http://codereview.appspot.com/4260053/diff/22001/java/gadgets/src/test/java/o... java/gadgets/src/test/java/org/apache/shindig/gadgets/http/HttpResponseTest.java:537: .create().getCacheExpiration())); actually this test seems too trivial, most likely it will just end up calling the setter and getter and return the same thing. u shud add a cache control max age header to test.
Sign in to reply to this message.
http://codereview.appspot.com/4260053/diff/22001/java/gadgets/src/main/java/o... File java/gadgets/src/main/java/org/apache/shindig/gadgets/http/HttpResponseBuilder.java (right): http://codereview.appspot.com/4260053/diff/22001/java/gadgets/src/main/java/o... java/gadgets/src/main/java/org/apache/shindig/gadgets/http/HttpResponseBuilder.java:237: cacheMaxAgeOverride = override; please add a incrementNumChanges() before returning here.. and also i forgot to add one in setCacheControlMaxAge. Could you add it there as well?
Sign in to reply to this message.
http://codereview.appspot.com/4260053/diff/22001/java/gadgets/src/main/java/o... File java/gadgets/src/main/java/org/apache/shindig/gadgets/http/HttpResponse.java (right): http://codereview.appspot.com/4260053/diff/22001/java/gadgets/src/main/java/o... java/gadgets/src/main/java/org/apache/shindig/gadgets/http/HttpResponse.java:22: import com.google.common.collect.ImmutableList; On 2011/03/08 19:54:38, gagan.goku wrote: > its okay to keep this wildcard. Run the optimize imports plugin in intellij, it > usually does the right thing. optimize imports for is configured to do this according to google style guide. Is the style different for shindig? http://codereview.appspot.com/4260053/diff/22001/java/gadgets/src/main/java/o... java/gadgets/src/main/java/org/apache/shindig/gadgets/http/HttpResponse.java:367: private long getInternalCacheMaxAge() { On 2011/03/08 19:54:38, gagan.goku wrote: > might as well get rid of this function, its just adding to confusion Done. http://codereview.appspot.com/4260053/diff/22001/java/gadgets/src/main/java/o... java/gadgets/src/main/java/org/apache/shindig/gadgets/http/HttpResponse.java:375: public long getCacheMaxAgeOverride() { On 2011/03/08 19:54:38, gagan.goku wrote: > please document in what cases this override is overriding something that is > being overridden. Done. http://codereview.appspot.com/4260053/diff/22001/java/gadgets/src/main/java/o... File java/gadgets/src/main/java/org/apache/shindig/gadgets/http/HttpResponseBuilder.java (right): http://codereview.appspot.com/4260053/diff/22001/java/gadgets/src/main/java/o... java/gadgets/src/main/java/org/apache/shindig/gadgets/http/HttpResponseBuilder.java:55: private long cacheMaxAgeOverride = -1; On 2011/03/08 19:54:38, gagan.goku wrote: > actually this is not really overriding the max age, this is the max-age in case > of noCache resources. > so maybe rename this variable to strictNoCacheMaxAge everywhere. Renamed to strictNoCacheExpiry to avoid confusion with max-age too. http://codereview.appspot.com/4260053/diff/22001/java/gadgets/src/main/java/o... java/gadgets/src/main/java/org/apache/shindig/gadgets/http/HttpResponseBuilder.java:237: cacheMaxAgeOverride = override; On 2011/03/09 05:05:43, nikhilmadan23 wrote: > please add a incrementNumChanges() before returning here.. and also i forgot to > add one in setCacheControlMaxAge. Could you add it there as well? Done.
Sign in to reply to this message.
http://codereview.appspot.com/4260053/diff/2010/java/gadgets/src/main/java/or... File java/gadgets/src/main/java/org/apache/shindig/gadgets/http/AbstractHttpCache.java (right): http://codereview.appspot.com/4260053/diff/2010/java/gadgets/src/main/java/or... java/gadgets/src/main/java/org/apache/shindig/gadgets/http/AbstractHttpCache.java:76: boolean storeStrictNoCacheResources = strictNoCacheResourceTtlInSeconds >= 0; () makes it more readable imo http://codereview.appspot.com/4260053/diff/2010/java/gadgets/src/main/java/or... java/gadgets/src/main/java/org/apache/shindig/gadgets/http/AbstractHttpCache.java:110: responseBuilder.setStrictNoCacheExpiry(strictNoCacheResourceTtlInSeconds); expiry usually means the time at which this entry will expire, meaning date + max-age But ur using it with max-age semantics. http://codereview.appspot.com/4260053/diff/2010/java/gadgets/src/main/java/or... File java/gadgets/src/main/java/org/apache/shindig/gadgets/http/HttpResponse.java (right): http://codereview.appspot.com/4260053/diff/2010/java/gadgets/src/main/java/or... java/gadgets/src/main/java/org/apache/shindig/gadgets/http/HttpResponse.java:169: private long strictNoCacheExpiry; Please document this var. Something like: The max-age of a resource that is strict no-cache. http://codereview.appspot.com/4260053/diff/2010/java/gadgets/src/main/java/or... java/gadgets/src/main/java/org/apache/shindig/gadgets/http/HttpResponse.java:362: return -1; you can move this statement out of else. http://codereview.appspot.com/4260053/diff/2010/java/gadgets/src/main/java/or... File java/gadgets/src/main/java/org/apache/shindig/gadgets/http/HttpResponseBuilder.java (right): http://codereview.appspot.com/4260053/diff/2010/java/gadgets/src/main/java/or... java/gadgets/src/main/java/org/apache/shindig/gadgets/http/HttpResponseBuilder.java:55: private long strictNoCacheExpiry = -1; same here, please document. http://codereview.appspot.com/4260053/diff/2010/java/gadgets/src/main/java/or... java/gadgets/src/main/java/org/apache/shindig/gadgets/http/HttpResponseBuilder.java:306: * the cache. This is not the max-age as stored in "private, max-age=.." but So whats wrong in keeping the max-age header with private resources? You can override the max-age in case of strict no-cache resources
Sign in to reply to this message.
http://codereview.appspot.com/4260053/diff/2010/java/gadgets/src/main/java/or... File java/gadgets/src/main/java/org/apache/shindig/gadgets/http/AbstractHttpCache.java (right): http://codereview.appspot.com/4260053/diff/2010/java/gadgets/src/main/java/or... java/gadgets/src/main/java/org/apache/shindig/gadgets/http/AbstractHttpCache.java:76: boolean storeStrictNoCacheResources = strictNoCacheResourceTtlInSeconds >= 0; On 2011/03/09 09:53:41, gagan.goku wrote: > () makes it more readable imo Done. http://codereview.appspot.com/4260053/diff/2010/java/gadgets/src/main/java/or... java/gadgets/src/main/java/org/apache/shindig/gadgets/http/AbstractHttpCache.java:110: responseBuilder.setStrictNoCacheExpiry(strictNoCacheResourceTtlInSeconds); On 2011/03/09 09:53:41, gagan.goku wrote: > expiry usually means the time at which this entry will expire, meaning date + > max-age > But ur using it with max-age semantics. Any name suggestion? Look at MapMaker().expiration(..), so using expiration for delay is common. Should I call it ttl? http://codereview.appspot.com/4260053/diff/2010/java/gadgets/src/main/java/or... File java/gadgets/src/main/java/org/apache/shindig/gadgets/http/HttpResponse.java (right): http://codereview.appspot.com/4260053/diff/2010/java/gadgets/src/main/java/or... java/gadgets/src/main/java/org/apache/shindig/gadgets/http/HttpResponse.java:169: private long strictNoCacheExpiry; On 2011/03/09 09:53:41, gagan.goku wrote: > Please document this var. Something like: > The max-age of a resource that is strict no-cache. Done. http://codereview.appspot.com/4260053/diff/2010/java/gadgets/src/main/java/or... java/gadgets/src/main/java/org/apache/shindig/gadgets/http/HttpResponse.java:362: return -1; On 2011/03/09 09:53:41, gagan.goku wrote: > you can move this statement out of else. Done. http://codereview.appspot.com/4260053/diff/2010/java/gadgets/src/main/java/or... File java/gadgets/src/main/java/org/apache/shindig/gadgets/http/HttpResponseBuilder.java (right): http://codereview.appspot.com/4260053/diff/2010/java/gadgets/src/main/java/or... java/gadgets/src/main/java/org/apache/shindig/gadgets/http/HttpResponseBuilder.java:55: private long strictNoCacheExpiry = -1; On 2011/03/09 09:53:41, gagan.goku wrote: > same here, please document. Done. http://codereview.appspot.com/4260053/diff/2010/java/gadgets/src/main/java/or... java/gadgets/src/main/java/org/apache/shindig/gadgets/http/HttpResponseBuilder.java:306: * the cache. This is not the max-age as stored in "private, max-age=.." but On 2011/03/09 09:53:41, gagan.goku wrote: > So whats wrong in keeping the max-age header with private resources? You can > override the max-age in case of strict no-cache resources Technically the amount we want to cache for and the amount we want to ask user to cache in browser are different. Hence the split.
Sign in to reply to this message.
Please take another look. All tests are passing, comments updated with the more details. The max-age usage is renamed to RefetchStrictNoCacheAfter everywhere including shindig.properties file.
Sign in to reply to this message.
Your patch description could mention the name of the variable (or flag atleast) instead of talking about it in an abstract fashion? http://codereview.appspot.com/4260053/diff/17003/java/common/conf/shindig.pro... File java/common/conf/shindig.properties (right): http://codereview.appspot.com/4260053/diff/17003/java/common/conf/shindig.pro... java/common/conf/shindig.properties:91: # Amount of time after which the entry in cache should be considered stale for an automated The comment does not mention strict no-cache resources at all. Aren't all of these applicable only for such resources - could you rephrase the comment to indicate it? http://codereview.appspot.com/4260053/diff/17003/java/common/conf/shindig.pro... java/common/conf/shindig.properties:92: # internal fetch. For any user facing request this value will not be considered. After this The sentence "For any user facing request, this value will not be considered" seems unnecessary and confusing. If there is a field called max-age, we might have had to clarify this, but with the renaming to "refetch-after", why should we even state this? http://codereview.appspot.com/4260053/diff/17003/java/common/conf/shindig.pro... java/common/conf/shindig.properties:93: # time for an internal fetch, it causes a refetch to the origin back end server. After this time for an internal fetch .... -> After the period specified by this value, a refetch for the resource is initiated. (Btw, the refetch is not automatic - correct? This comment is misleading in that sense - can you rephrase?) http://codereview.appspot.com/4260053/diff/17003/java/gadgets/src/main/java/o... File java/gadgets/src/main/java/org/apache/shindig/gadgets/http/AbstractHttpCache.java (right): http://codereview.appspot.com/4260053/diff/17003/java/gadgets/src/main/java/o... java/gadgets/src/main/java/org/apache/shindig/gadgets/http/AbstractHttpCache.java:37: * internal to shindig from triggering too many backend fetches. Especially comes to play for fetch too many backend fetches -> lots of backend fetches. http://codereview.appspot.com/4260053/diff/17003/java/gadgets/src/main/java/o... java/gadgets/src/main/java/org/apache/shindig/gadgets/http/AbstractHttpCache.java:39: * need to explicitly check if the content is strictNoCache or not. DefaultRequestPipeline does this we need to explicitly check if the content is strictNoCache or not -> we need to explicitly check if the content is strictNoCache or not before using this content. http://codereview.appspot.com/4260053/diff/17003/java/gadgets/src/main/java/o... java/gadgets/src/main/java/org/apache/shindig/gadgets/http/AbstractHttpCache.java:56: // considered as expired as long as they haven't expired otherwise. The last sentence baffled me - could you explain what could constitute "expired otherwise" in this case? http://codereview.appspot.com/4260053/diff/17003/java/gadgets/src/main/java/o... java/gadgets/src/main/java/org/apache/shindig/gadgets/http/AbstractHttpCache.java:58: public static long REFETCH_STRICT_NO_CACHE_AFTER_DEFAULT = - 1L; Space b/w - and 1. http://codereview.appspot.com/4260053/diff/17003/java/gadgets/src/main/java/o... java/gadgets/src/main/java/org/apache/shindig/gadgets/http/AbstractHttpCache.java:102: * Override the TTL for storing strictly no cache content. strictly no cache -> strict no cache http://codereview.appspot.com/4260053/diff/17003/java/gadgets/src/test/java/o... File java/gadgets/src/test/java/org/apache/shindig/gadgets/http/AbstractHttpCacheTest.java (left): http://codereview.appspot.com/4260053/diff/17003/java/gadgets/src/test/java/o... java/gadgets/src/test/java/org/apache/shindig/gadgets/http/AbstractHttpCacheTest.java:465: .buildStrictNoCacheHttpResponse(new HttpRequest(DEFAULT_URI), response).create(); Could you leave out this change (of removing request form buildStrictNoCacheHttpResponse) from this patch? One of Nikhil's less complicated patches does this anyways.
Sign in to reply to this message.
cl is looking good now. Thanks for patiently making all the changes i imposed on u (please dont beat me in tt next time). Small nits: http://codereview.appspot.com/4260053/diff/19009/java/gadgets/src/main/java/o... File java/gadgets/src/main/java/org/apache/shindig/gadgets/http/HttpResponse.java (right): http://codereview.appspot.com/4260053/diff/19009/java/gadgets/src/main/java/o... java/gadgets/src/main/java/org/apache/shindig/gadgets/http/HttpResponse.java:352: long maxAge = isStrictNoCache() ? getStrictNoCacheExpiry() : getCacheControlMaxAge(); rename to getStrictNoCacheMaxAge or getStrictNoCacheTtl ? http://codereview.appspot.com/4260053/diff/17003/java/common/conf/shindig.pro... File java/common/conf/shindig.properties (right): http://codereview.appspot.com/4260053/diff/17003/java/common/conf/shindig.pro... java/common/conf/shindig.properties:91: # Amount of time after which the entry in cache should be considered stale for an automated lets not say anything about stale. If you rephrase it to something like, time after which automated internal fetcher should refetch strict no cache resources so that it knows of them beforehand and does not proxy these through via proxy servlet, it will be much clearer to even novice ppl like me. http://codereview.appspot.com/4260053/diff/17003/java/gadgets/src/main/java/o... File java/gadgets/src/main/java/org/apache/shindig/gadgets/http/AbstractHttpCache.java (right): http://codereview.appspot.com/4260053/diff/17003/java/gadgets/src/main/java/o... java/gadgets/src/main/java/org/apache/shindig/gadgets/http/AbstractHttpCache.java:36: * and not the original content or other headers. This is used primarily for automatic fetches Nicely put, requesting you to put this on a newline, with all caps NOTE: at the beginning of the line, and probably similar comment in RequestPipeline interface (you can put a short version and put a @see pointer to this file). http://codereview.appspot.com/4260053/diff/17003/java/gadgets/src/main/java/o... File java/gadgets/src/main/java/org/apache/shindig/gadgets/http/HttpResponse.java (right): http://codereview.appspot.com/4260053/diff/17003/java/gadgets/src/main/java/o... java/gadgets/src/main/java/org/apache/shindig/gadgets/http/HttpResponse.java:362: return -1; u can move the return -1 outside the else. http://codereview.appspot.com/4260053/diff/17003/java/gadgets/src/test/java/o... File java/gadgets/src/test/java/org/apache/shindig/gadgets/http/AbstractHttpCacheTest.java (right): http://codereview.appspot.com/4260053/diff/17003/java/gadgets/src/test/java/o... java/gadgets/src/test/java/org/apache/shindig/gadgets/http/AbstractHttpCacheTest.java:453: assertEquals("private", builtResponse.getHeader("Cache-Control")); this does not seem right http://codereview.appspot.com/4260053/diff/14009/java/gadgets/src/test/java/o... java/gadgets/src/test/java/org/apache/shindig/gadgets/http/AbstractHttpCacheTest.java:59: extendedStrictNoCacheTtlCache.setRefetchStrictNoCacheAfter(86400L); shouldnt this be 86400*1000L (im assuming u wanted to set it as 1 day) http://codereview.appspot.com/4260053/diff/14009/java/gadgets/src/test/java/o... File java/gadgets/src/test/java/org/apache/shindig/gadgets/http/HttpResponseTest.java (right): http://codereview.appspot.com/4260053/diff/14009/java/gadgets/src/test/java/o... java/gadgets/src/test/java/org/apache/shindig/gadgets/http/HttpResponseTest.java:534: public void testCacheExpirationForStrictNoCacheResponse() throws Exception { add a max-age to this response as well. Then ul have 2 tests, max-age < refetch-after max-age >= refetch-after For both, isStale() should return true. and cacheExpiration -1 http://codereview.appspot.com/4260053/diff/14009/java/gadgets/src/test/java/o... java/gadgets/src/test/java/org/apache/shindig/gadgets/http/HttpResponseTest.java:536: new HttpResponseBuilder().setStrictNoCache() please break over newlines, it makes it more readable new HttpResponseBuilder() .setStrictNoCache() .setRefetchStrictNoCacheAfterMs(10000) .create() .getCacheExpiration() http://codereview.appspot.com/4260053/diff/14009/java/gadgets/src/test/java/o... java/gadgets/src/test/java/org/apache/shindig/gadgets/http/HttpResponseTest.java:542: assertEquals(false, whats current ? http://codereview.appspot.com/4260053/diff/14009/java/gadgets/src/test/java/o... File java/gadgets/src/test/java/org/apache/shindig/gadgets/rewrite/CacheEnforcementVisitorTest.java (right): http://codereview.appspot.com/4260053/diff/14009/java/gadgets/src/test/java/o... java/gadgets/src/test/java/org/apache/shindig/gadgets/rewrite/CacheEnforcementVisitorTest.java:65: cache.setRefetchStrictNoCacheAfterMs(86400L); 86400*1000 ?
Sign in to reply to this message.
http://codereview.appspot.com/4260053/diff/14009/java/gadgets/src/main/java/o... File java/gadgets/src/main/java/org/apache/shindig/gadgets/http/AbstractHttpCache.java (left): http://codereview.appspot.com/4260053/diff/14009/java/gadgets/src/main/java/o... java/gadgets/src/main/java/org/apache/shindig/gadgets/http/AbstractHttpCache.java:76: if (storeStrictNoCacheResources && response.isStrictNoCache()) { Why do we no longer need this check for refetchStrictNoCacheAfterMs >= 0? Wouldn't we now build a cleared-up response, even if the refetchStrictNoCacheAfterMs is -1? http://codereview.appspot.com/4260053/diff/14009/java/gadgets/src/main/java/o... File java/gadgets/src/main/java/org/apache/shindig/gadgets/http/AbstractHttpCache.java (right): http://codereview.appspot.com/4260053/diff/14009/java/gadgets/src/main/java/o... java/gadgets/src/main/java/org/apache/shindig/gadgets/http/AbstractHttpCache.java:82: public static long REFETCH_STRICT_NO_CACHE_AFTER_MS_DEFAULT = - 1L; Space after - and 1 not removed. http://codereview.appspot.com/4260053/diff/14009/java/gadgets/src/main/java/o... java/gadgets/src/main/java/org/apache/shindig/gadgets/http/AbstractHttpCache.java:125: public void setRefetchStrictNoCacheAfterMs(long refetchStrictNoCacheAfterMs) { This seems to be used only from AbstractHttpCacheTest - should we make it private and VisibleForTesting for clarity of why this method even exists? http://codereview.appspot.com/4260053/diff/14009/java/gadgets/src/main/java/o... java/gadgets/src/main/java/org/apache/shindig/gadgets/http/AbstractHttpCache.java:134: responseBuilder.setRefetchStrictNoCacheAfterMs(refetchStrictNoCacheAfterMs); Instead of bringing responseBuilder into this refetch logic, why can't we directly set response.setRefetchStrictNoCacheAfterMs after line 119 and return that modified response?
Sign in to reply to this message.
Some comments were obsolete and in old patch, so all of them may not be replied. But I hope all are done. http://codereview.appspot.com/4260053/diff/19009/java/gadgets/src/main/java/o... File java/gadgets/src/main/java/org/apache/shindig/gadgets/http/HttpResponse.java (right): http://codereview.appspot.com/4260053/diff/19009/java/gadgets/src/main/java/o... java/gadgets/src/main/java/org/apache/shindig/gadgets/http/HttpResponse.java:352: long maxAge = isStrictNoCache() ? getStrictNoCacheExpiry() : getCacheControlMaxAge(); On 2011/03/10 12:18:06, gagan.goku wrote: > rename to getStrictNoCacheMaxAge or getStrictNoCacheTtl ? Obsolete comment. http://codereview.appspot.com/4260053/diff/17003/java/gadgets/src/main/java/o... File java/gadgets/src/main/java/org/apache/shindig/gadgets/http/AbstractHttpCache.java (right): http://codereview.appspot.com/4260053/diff/17003/java/gadgets/src/main/java/o... java/gadgets/src/main/java/org/apache/shindig/gadgets/http/AbstractHttpCache.java:36: * and not the original content or other headers. This is used primarily for automatic fetches On 2011/03/10 12:18:06, gagan.goku wrote: > Nicely put, requesting you to put this on a newline, with all caps NOTE: at the > beginning of the line, > and probably similar comment in RequestPipeline interface (you can put a short > version and put a @see pointer to this file). Done. http://codereview.appspot.com/4260053/diff/17003/java/gadgets/src/main/java/o... java/gadgets/src/main/java/org/apache/shindig/gadgets/http/AbstractHttpCache.java:37: * internal to shindig from triggering too many backend fetches. Especially comes to play for fetch On 2011/03/10 10:33:57, anupama.dutta wrote: > too many backend fetches -> lots of backend fetches. Done. http://codereview.appspot.com/4260053/diff/17003/java/gadgets/src/main/java/o... java/gadgets/src/main/java/org/apache/shindig/gadgets/http/AbstractHttpCache.java:58: public static long REFETCH_STRICT_NO_CACHE_AFTER_DEFAULT = - 1L; On 2011/03/10 10:33:57, anupama.dutta wrote: > Space b/w - and 1. Done. http://codereview.appspot.com/4260053/diff/17003/java/gadgets/src/main/java/o... java/gadgets/src/main/java/org/apache/shindig/gadgets/http/AbstractHttpCache.java:102: * Override the TTL for storing strictly no cache content. On 2011/03/10 10:33:57, anupama.dutta wrote: > strictly no cache -> strict no cache Done. http://codereview.appspot.com/4260053/diff/17003/java/gadgets/src/main/java/o... File java/gadgets/src/main/java/org/apache/shindig/gadgets/http/HttpResponse.java (right): http://codereview.appspot.com/4260053/diff/17003/java/gadgets/src/main/java/o... java/gadgets/src/main/java/org/apache/shindig/gadgets/http/HttpResponse.java:362: return -1; On 2011/03/10 12:18:06, gagan.goku wrote: > u can move the return -1 outside the else. Done. http://codereview.appspot.com/4260053/diff/17003/java/gadgets/src/test/java/o... File java/gadgets/src/test/java/org/apache/shindig/gadgets/http/AbstractHttpCacheTest.java (right): http://codereview.appspot.com/4260053/diff/17003/java/gadgets/src/test/java/o... java/gadgets/src/test/java/org/apache/shindig/gadgets/http/AbstractHttpCacheTest.java:453: assertEquals("private", builtResponse.getHeader("Cache-Control")); On 2011/03/10 12:18:06, gagan.goku wrote: > this does not seem right Why? http://codereview.appspot.com/4260053/diff/14009/java/gadgets/src/main/java/o... File java/gadgets/src/main/java/org/apache/shindig/gadgets/http/AbstractHttpCache.java (left): http://codereview.appspot.com/4260053/diff/14009/java/gadgets/src/main/java/o... java/gadgets/src/main/java/org/apache/shindig/gadgets/http/AbstractHttpCache.java:76: if (storeStrictNoCacheResources && response.isStrictNoCache()) { On 2011/03/10 15:09:59, anupama.dutta wrote: > Why do we no longer need this check for refetchStrictNoCacheAfterMs >= 0? > Wouldn't we now build a cleared-up response, even if the > refetchStrictNoCacheAfterMs is -1? This check was never needed, previous if would not have passed. http://codereview.appspot.com/4260053/diff/14009/java/gadgets/src/main/java/o... File java/gadgets/src/main/java/org/apache/shindig/gadgets/http/AbstractHttpCache.java (right): http://codereview.appspot.com/4260053/diff/14009/java/gadgets/src/main/java/o... java/gadgets/src/main/java/org/apache/shindig/gadgets/http/AbstractHttpCache.java:82: public static long REFETCH_STRICT_NO_CACHE_AFTER_MS_DEFAULT = - 1L; On 2011/03/10 15:09:59, anupama.dutta wrote: > Space after - and 1 not removed. Done. http://codereview.appspot.com/4260053/diff/14009/java/gadgets/src/main/java/o... java/gadgets/src/main/java/org/apache/shindig/gadgets/http/AbstractHttpCache.java:125: public void setRefetchStrictNoCacheAfterMs(long refetchStrictNoCacheAfterMs) { On 2011/03/10 15:09:59, anupama.dutta wrote: > This seems to be used only from AbstractHttpCacheTest - should we make it > private and VisibleForTesting for clarity of why this method even exists? Well rest of the places we treated it as public. I will mark all of them as package-private, because even though they are used finally only for testing, they are used cross package not just in test. http://codereview.appspot.com/4260053/diff/14009/java/gadgets/src/main/java/o... java/gadgets/src/main/java/org/apache/shindig/gadgets/http/AbstractHttpCache.java:134: responseBuilder.setRefetchStrictNoCacheAfterMs(refetchStrictNoCacheAfterMs); On 2011/03/10 15:09:59, anupama.dutta wrote: > Instead of bringing responseBuilder into this refetch logic, why can't we > directly set response.setRefetchStrictNoCacheAfterMs after line 119 and return > that modified response? HttpResponse is an immutable object, immutables are nice. I want to keep it that way. http://codereview.appspot.com/4260053/diff/14009/java/gadgets/src/test/java/o... File java/gadgets/src/test/java/org/apache/shindig/gadgets/http/AbstractHttpCacheTest.java (right): http://codereview.appspot.com/4260053/diff/14009/java/gadgets/src/test/java/o... java/gadgets/src/test/java/org/apache/shindig/gadgets/http/AbstractHttpCacheTest.java:59: extendedStrictNoCacheTtlCache.setRefetchStrictNoCacheAfterMs(86400L); On 2011/03/10 12:18:06, gagan.goku wrote: > shouldnt this be 86400*1000L (im assuming u wanted to set it as 1 day) Done. http://codereview.appspot.com/4260053/diff/14009/java/gadgets/src/test/java/o... File java/gadgets/src/test/java/org/apache/shindig/gadgets/http/HttpResponseTest.java (right): http://codereview.appspot.com/4260053/diff/14009/java/gadgets/src/test/java/o... java/gadgets/src/test/java/org/apache/shindig/gadgets/http/HttpResponseTest.java:534: public void testCacheExpirationForStrictNoCacheResponse() throws Exception { On 2011/03/10 12:18:06, gagan.goku wrote: > add a max-age to this response as well. Then ul have 2 tests, > max-age < refetch-after > max-age >= refetch-after > For both, isStale() should return true. and cacheExpiration -1 Done. http://codereview.appspot.com/4260053/diff/14009/java/gadgets/src/test/java/o... java/gadgets/src/test/java/org/apache/shindig/gadgets/http/HttpResponseTest.java:536: new HttpResponseBuilder().setStrictNoCache() On 2011/03/10 12:18:06, gagan.goku wrote: > please break over newlines, it makes it more readable > > new HttpResponseBuilder() > .setStrictNoCache() > .setRefetchStrictNoCacheAfterMs(10000) > .create() > .getCacheExpiration() Done. http://codereview.appspot.com/4260053/diff/14009/java/gadgets/src/test/java/o... java/gadgets/src/test/java/org/apache/shindig/gadgets/http/HttpResponseTest.java:542: assertEquals(false, On 2011/03/10 12:18:06, gagan.goku wrote: > whats current ? current-time.
Sign in to reply to this message.
http://codereview.appspot.com/4260053/diff/14009/java/gadgets/src/main/java/o... File java/gadgets/src/main/java/org/apache/shindig/gadgets/http/AbstractHttpCache.java (left): http://codereview.appspot.com/4260053/diff/14009/java/gadgets/src/main/java/o... java/gadgets/src/main/java/org/apache/shindig/gadgets/http/AbstractHttpCache.java:76: if (storeStrictNoCacheResources && response.isStrictNoCache()) { On 2011/03/10 15:58:26, atulvasu wrote: > On 2011/03/10 15:09:59, anupama.dutta wrote: > > Why do we no longer need this check for refetchStrictNoCacheAfterMs >= 0? > > Wouldn't we now build a cleared-up response, even if the > > refetchStrictNoCacheAfterMs is -1? > > This check was never needed, previous if would not have passed. Didn't get this. In case of strict-no-cache responses (with strictNoCacheResourceTtlInSeconds = -1), the isCacheable check would still have returned true - this was the original flow used by all current clients of this code, if I understand right. Why do you say that the previoud if would not have passed - please check this case with Nikhil, since I remember discussing this in a lot of detail when he added this code.
Sign in to reply to this message.
http://codereview.appspot.com/4260053/diff/14009/java/gadgets/src/main/java/o... File java/gadgets/src/main/java/org/apache/shindig/gadgets/http/AbstractHttpCache.java (left): http://codereview.appspot.com/4260053/diff/14009/java/gadgets/src/main/java/o... java/gadgets/src/main/java/org/apache/shindig/gadgets/http/AbstractHttpCache.java:76: if (storeStrictNoCacheResources && response.isStrictNoCache()) { On 2011/03/10 16:28:04, anupama.dutta wrote: > On 2011/03/10 15:58:26, atulvasu wrote: > > On 2011/03/10 15:09:59, anupama.dutta wrote: > > > Why do we no longer need this check for refetchStrictNoCacheAfterMs >= 0? > > > Wouldn't we now build a cleared-up response, even if the > > > refetchStrictNoCacheAfterMs is -1? > > > > This check was never needed, previous if would not have passed. > > Didn't get this. In case of strict-no-cache responses (with > strictNoCacheResourceTtlInSeconds = -1), the isCacheable check would still have > returned true - this was the original flow used by all current clients of this > code, if I understand right. Why do you say that the previoud if would not have > passed - please check this case with Nikhil, since I remember discussing this in > a lot of detail when he added this code. Ideally it would not have been required.. But since isCacheable can return true, even if its strictNoCache(for example if request.getCacheTtl != -1), this check is essential to maintain original behavior. http://codereview.appspot.com/4260053/diff/14009/java/gadgets/src/main/java/o... File java/gadgets/src/main/java/org/apache/shindig/gadgets/http/HttpResponse.java (right): http://codereview.appspot.com/4260053/diff/14009/java/gadgets/src/main/java/o... java/gadgets/src/main/java/org/apache/shindig/gadgets/http/HttpResponse.java:366: * @see {HttpResponseBuilder.getRefetchStrictNoCacheAfterMs}. nit: there isn't any comment there..maybe point to AbstractHttpCache instead http://codereview.appspot.com/4260053/diff/13010/java/common/conf/shindig.pro... File java/common/conf/shindig.properties (right): http://codereview.appspot.com/4260053/diff/13010/java/common/conf/shindig.pro... java/common/conf/shindig.properties:91: # Amount of time after which the entry in cache should be considered for a refetch for an nit : for a non-userfacing http://codereview.appspot.com/4260053/diff/13010/java/gadgets/src/main/java/o... File java/gadgets/src/main/java/org/apache/shindig/gadgets/http/AbstractHttpCache.java (right): http://codereview.appspot.com/4260053/diff/13010/java/gadgets/src/main/java/o... java/gadgets/src/main/java/org/apache/shindig/gadgets/http/AbstractHttpCache.java:38: * This is used primarily for automatic fetches internal to shindig from triggering lots of for preventing automatic fetches? http://codereview.appspot.com/4260053/diff/13010/java/gadgets/src/main/java/o... java/gadgets/src/main/java/org/apache/shindig/gadgets/http/AbstractHttpCache.java:58: * For non user facing requests, www.example.com/private.html is considered a private and will not nit: considered as http://codereview.appspot.com/4260053/diff/13010/java/gadgets/src/main/java/o... java/gadgets/src/main/java/org/apache/shindig/gadgets/http/AbstractHttpCache.java:61: * For user facing request, response.isStale() is always true,and will be fetched even before nit: user facing requests and a space after is http://codereview.appspot.com/4260053/diff/13010/java/gadgets/src/main/java/o... java/gadgets/src/main/java/org/apache/shindig/gadgets/http/AbstractHttpCache.java:98: (!cached.isStrictNoCache() || refetchStrictNoCacheAfterMs > 0)) { not related to this cl..but a bug in my code..could you change to refetchStrictNoCacheAfterMs >= 0 http://codereview.appspot.com/4260053/diff/13010/java/gadgets/src/test/java/o... File java/gadgets/src/test/java/org/apache/shindig/gadgets/http/AbstractHttpCacheTest.java (right): http://codereview.appspot.com/4260053/diff/13010/java/gadgets/src/test/java/o... java/gadgets/src/test/java/org/apache/shindig/gadgets/http/AbstractHttpCacheTest.java:53: // Cache designed to return 86400ms for strictNoCacheResourceTtl. for refetchStrictNoCacheAfterMs
Sign in to reply to this message.
http://codereview.appspot.com/4260053/diff/14009/java/gadgets/src/main/java/o... File java/gadgets/src/main/java/org/apache/shindig/gadgets/http/AbstractHttpCache.java (left): http://codereview.appspot.com/4260053/diff/14009/java/gadgets/src/main/java/o... java/gadgets/src/main/java/org/apache/shindig/gadgets/http/AbstractHttpCache.java:76: if (storeStrictNoCacheResources && response.isStrictNoCache()) { On 2011/03/10 16:47:41, nikhilmadan23 wrote: > On 2011/03/10 16:28:04, anupama.dutta wrote: > > On 2011/03/10 15:58:26, atulvasu wrote: > > > On 2011/03/10 15:09:59, anupama.dutta wrote: > > > > Why do we no longer need this check for refetchStrictNoCacheAfterMs >= 0? > > > > Wouldn't we now build a cleared-up response, even if the > > > > refetchStrictNoCacheAfterMs is -1? > > > > > > This check was never needed, previous if would not have passed. > > > > Didn't get this. In case of strict-no-cache responses (with > > strictNoCacheResourceTtlInSeconds = -1), the isCacheable check would still > have > > returned true - this was the original flow used by all current clients of this > > code, if I understand right. Why do you say that the previoud if would not > have > > passed - please check this case with Nikhil, since I remember discussing this > in > > a lot of detail when he added this code. > > Ideally it would not have been required.. But since isCacheable can return true, > even if its strictNoCache(for example if request.getCacheTtl != -1), this check > is essential to maintain original behavior. :( Yes, fixed. http://codereview.appspot.com/4260053/diff/14009/java/gadgets/src/main/java/o... File java/gadgets/src/main/java/org/apache/shindig/gadgets/http/HttpResponse.java (right): http://codereview.appspot.com/4260053/diff/14009/java/gadgets/src/main/java/o... java/gadgets/src/main/java/org/apache/shindig/gadgets/http/HttpResponse.java:366: * @see {HttpResponseBuilder.getRefetchStrictNoCacheAfterMs}. On 2011/03/10 16:47:41, nikhilmadan23 wrote: > nit: there isn't any comment there..maybe point to AbstractHttpCache instead Removed completely, because it is just a setter. http://codereview.appspot.com/4260053/diff/13010/java/common/conf/shindig.pro... File java/common/conf/shindig.properties (right): http://codereview.appspot.com/4260053/diff/13010/java/common/conf/shindig.pro... java/common/conf/shindig.properties:91: # Amount of time after which the entry in cache should be considered for a refetch for an On 2011/03/10 16:47:41, nikhilmadan23 wrote: > nit : for a non-userfacing Already there, right? http://codereview.appspot.com/4260053/diff/13010/java/gadgets/src/main/java/o... File java/gadgets/src/main/java/org/apache/shindig/gadgets/http/AbstractHttpCache.java (right): http://codereview.appspot.com/4260053/diff/13010/java/gadgets/src/main/java/o... java/gadgets/src/main/java/org/apache/shindig/gadgets/http/AbstractHttpCache.java:38: * This is used primarily for automatic fetches internal to shindig from triggering lots of On 2011/03/10 16:47:41, nikhilmadan23 wrote: > for preventing automatic fetches? well called it back-end fetch to distinguish from cache fetches. http://codereview.appspot.com/4260053/diff/13010/java/gadgets/src/main/java/o... java/gadgets/src/main/java/org/apache/shindig/gadgets/http/AbstractHttpCache.java:58: * For non user facing requests, www.example.com/private.html is considered a private and will not On 2011/03/10 16:47:41, nikhilmadan23 wrote: > nit: considered as Done. http://codereview.appspot.com/4260053/diff/13010/java/gadgets/src/main/java/o... java/gadgets/src/main/java/org/apache/shindig/gadgets/http/AbstractHttpCache.java:61: * For user facing request, response.isStale() is always true,and will be fetched even before On 2011/03/10 16:47:41, nikhilmadan23 wrote: > nit: user facing requests and a space after is Space after , you mean? http://codereview.appspot.com/4260053/diff/13010/java/gadgets/src/main/java/o... java/gadgets/src/main/java/org/apache/shindig/gadgets/http/AbstractHttpCache.java:98: (!cached.isStrictNoCache() || refetchStrictNoCacheAfterMs > 0)) { On 2011/03/10 16:47:41, nikhilmadan23 wrote: > not related to this cl..but a bug in my code..could you change to > refetchStrictNoCacheAfterMs >= 0 Done. http://codereview.appspot.com/4260053/diff/13010/java/gadgets/src/test/java/o... File java/gadgets/src/test/java/org/apache/shindig/gadgets/http/AbstractHttpCacheTest.java (right): http://codereview.appspot.com/4260053/diff/13010/java/gadgets/src/test/java/o... java/gadgets/src/test/java/org/apache/shindig/gadgets/http/AbstractHttpCacheTest.java:53: // Cache designed to return 86400ms for strictNoCacheResourceTtl. On 2011/03/10 16:47:41, nikhilmadan23 wrote: > for refetchStrictNoCacheAfterMs Done.
Sign in to reply to this message.
lgtm http://codereview.appspot.com/4260053/diff/27001/java/common/conf/shindig.pro... File java/common/conf/shindig.properties (right): http://codereview.appspot.com/4260053/diff/27001/java/common/conf/shindig.pro... java/common/conf/shindig.properties:91: # Amount of time after which the entry in cache should be considered for a refetch for an s/an/a http://codereview.appspot.com/4260053/diff/27001/java/gadgets/src/main/java/o... File java/gadgets/src/main/java/org/apache/shindig/gadgets/http/AbstractHttpCache.java (right): http://codereview.appspot.com/4260053/diff/27001/java/gadgets/src/main/java/o... java/gadgets/src/main/java/org/apache/shindig/gadgets/http/AbstractHttpCache.java:65: * isStale() = false always, for all time >=0 nit : space after >=
Sign in to reply to this message.
http://codereview.appspot.com/4260053/diff/27001/java/common/conf/shindig.pro... File java/common/conf/shindig.properties (right): http://codereview.appspot.com/4260053/diff/27001/java/common/conf/shindig.pro... java/common/conf/shindig.properties:91: # Amount of time after which the entry in cache should be considered for a refetch for an On 2011/03/11 12:01:28, nikhilmadan23 wrote: > s/an/a Done. http://codereview.appspot.com/4260053/diff/27001/java/gadgets/src/main/java/o... File java/gadgets/src/main/java/org/apache/shindig/gadgets/http/AbstractHttpCache.java (right): http://codereview.appspot.com/4260053/diff/27001/java/gadgets/src/main/java/o... java/gadgets/src/main/java/org/apache/shindig/gadgets/http/AbstractHttpCache.java:65: * isStale() = false always, for all time >=0 On 2011/03/11 12:01:28, nikhilmadan23 wrote: > nit : space after >= Done.
Sign in to reply to this message.
Awesome cl. committing now.
Sign in to reply to this message.
Build looks good. Committed as r1080673.
Sign in to reply to this message.
thanks gagan. On Fri, Mar 11, 2011 at 10:42 PM, <gagan.goku@gmail.com> wrote: > Build looks good. > Committed as r1080673. > > > http://codereview.appspot.com/4260053/ >
Sign in to reply to this message.
|
