Proxy should use resource TTL by default

It seems that Proxy would ignore external resource ttl and would use the default (an hour).
Update the code

[gagan.goku, 2010-09-30 01:57:52]

Hi Ziv

Kuntal has more idea about caching so adding her to the code review.
Also, can we document some of the methods and member variables on ProxyUriBase
in this cl.
For example, ProxyUriBase has:
  public Integer translateStatusRefresh(int longVal, int defaultVal)
      throws GadgetException {

Would be nice if we could refactor this to say:
// longVal = long expiry value for a versioned url.
// originalResourceExpriresTTL = the ttl expiry time of the resource being
proxied.
public Integer translateStatusRefresh(int longVal, int
originalResourceExpriresTTL)
      throws GadgetException {

Also a comment explaining why we don't want to serve out non-zero cache expiry
for a invalid version resource will be very helpful.
The comment could go something like:
because the server might have an older version in its cache (server might be out
of sync / original resource had incorrect caching header and the content changed
before its advertised expiry time) and would serve out possibly old content with
a non zero cache expiry. Public proxies can cache this and provide incorrect
resources to many users.
To minimize such inconsistencies, its best to serve no-cache,max-age=0 cache
header for invalid versions.

http://codereview.appspot.com/2298042/diff/1/java/gadgets/src/main/java/org/a...
File
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/ProxyHandler.java
(right):

http://codereview.appspot.com/2298042/diff/1/java/gadgets/src/main/java/org/a...
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/ProxyHandler.java:112:
proxyUri.translateStatusRefresh(LONG_LIVED_REFRESH, (int) (results.getCacheTtl()
/ 1000)),
shouldnt you also be doing the same in this file:
http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/main/java/org/...

[gagan.goku, 2010-09-30 01:59:22]

+ cool-shindig-committers@googlegroups.com

On Thu, Sep 30, 2010 at 7:27 AM, <gagan.goku@gmail.com> wrote:

> Hi Ziv
>
> Kuntal has more idea about caching so adding her to the code review.
> Also, can we document some of the methods and member variables on
> ProxyUriBase in this cl.
> For example, ProxyUriBase has:
>  public Integer translateStatusRefresh(int longVal, int defaultVal)
>      throws GadgetException {
>
> Would be nice if we could refactor this to say:
> // longVal = long expiry value for a versioned url.
> // originalResourceExpriresTTL = the ttl expiry time of the resource
> being proxied.
> public Integer translateStatusRefresh(int longVal, int
> originalResourceExpriresTTL)
>      throws GadgetException {
>
> Also a comment explaining why we don't want to serve out non-zero cache
> expiry for a invalid version resource will be very helpful.
> The comment could go something like:
> because the server might have an older version in its cache (server
> might be out of sync / original resource had incorrect caching header
> and the content changed before its advertised expiry time) and would
> serve out possibly old content with a non zero cache expiry. Public
> proxies can cache this and provide incorrect resources to many users.
> To minimize such inconsistencies, its best to serve no-cache,max-age=0
> cache header for invalid versions.
>
>
>
>
>
http://codereview.appspot.com/2298042/diff/1/java/gadgets/src/main/java/org/a...
> File
>
>
>
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/ProxyHandler.java
> (right):
>
>
>
http://codereview.appspot.com/2298042/diff/1/java/gadgets/src/main/java/org/a...
>
>
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/ProxyHandler.java:112:
> proxyUri.translateStatusRefresh(LONG_LIVED_REFRESH, (int)
> (results.getCacheTtl() / 1000)),
> shouldnt you also be doing the same in this file:
>
>
http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/main/java/org/...
>
>
> http://codereview.appspot.com/2298042/
>

[zhoresh, 2010-09-30 18:53:48]

Added fix for concat and add comments

[zhoresh, 2010-09-30 18:58:51]

On 2010/09/30 01:57:52, gagan.goku wrote:
> Hi Ziv
> 
> Kuntal has more idea about caching so adding her to the code review.
> Also, can we document some of the methods and member variables on ProxyUriBase
> in this cl.
> For example, ProxyUriBase has:
>   public Integer translateStatusRefresh(int longVal, int defaultVal)
>       throws GadgetException {
> 
> Would be nice if we could refactor this to say:
> // longVal = long expiry value for a versioned url.
> // originalResourceExpriresTTL = the ttl expiry time of the resource being
> proxied.
> public Integer translateStatusRefresh(int longVal, int
> originalResourceExpriresTTL)
>       throws GadgetException {
> 
> Also a comment explaining why we don't want to serve out non-zero cache expiry
> for a invalid version resource will be very helpful.
> The comment could go something like:
> because the server might have an older version in its cache (server might be
out
> of sync / original resource had incorrect caching header and the content
changed
> before its advertised expiry time) and would serve out possibly old content
with
> a non zero cache expiry. Public proxies can cache this and provide incorrect
> resources to many users.
> To minimize such inconsistencies, its best to serve no-cache,max-age=0 cache
> header for invalid versions.

Added comments

> 
>
http://codereview.appspot.com/2298042/diff/1/java/gadgets/src/main/java/org/a...
> File
>
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/ProxyHandler.java
> (right):
> 
>
http://codereview.appspot.com/2298042/diff/1/java/gadgets/src/main/java/org/a...
>
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/ProxyHandler.java:112:
> proxyUri.translateStatusRefresh(LONG_LIVED_REFRESH, (int)
(results.getCacheTtl()
> / 1000)),
> shouldnt you also be doing the same in this file:
>
http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/main/java/org/...

Good catch, I updated the concat as well.

Thanks!

[Kuntal Loya, 2010-09-30 19:58:14]

Thanks Ziv for making this change.
I have a couple of doubts though -

http://codereview.appspot.com/2298042/diff/8001/java/gadgets/src/main/java/or...
File
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/ConcatProxyServlet.java
(right):

http://codereview.appspot.com/2298042/diff/8001/java/gadgets/src/main/java/or...
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/ConcatProxyServlet.java:216:
minTtl = Math.min(minTtl, httpResp.getCacheTtl());
Just curious, is there a particular reason why the cacheTtl for HttpResponse is
in ms while for HttpRequest is in seconds?
The max-age in response headers is supposed to be in seconds.

http://codereview.appspot.com/2298042/diff/8001/java/gadgets/src/main/java/or...
File
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/ProxyHandler.java
(right):

http://codereview.appspot.com/2298042/diff/8001/java/gadgets/src/main/java/or...
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/ProxyHandler.java:112:
proxyUri.translateStatusRefresh(LONG_LIVED_REFRESH, (int) (results.getCacheTtl()
/ 1000)),
Shouldn't we use the DEFAULT_REFRESH if the response is no-cache or has no cache
headers?
Similarly for ConcatProxyHandler

http://codereview.appspot.com/2298042/diff/8001/java/gadgets/src/main/java/or...
File java/gadgets/src/main/java/org/apache/shindig/gadgets/uri/ProxyUriBase.java
(right):

http://codereview.appspot.com/2298042/diff/8001/java/gadgets/src/main/java/or...
java/gadgets/src/main/java/org/apache/shindig/gadgets/uri/ProxyUriBase.java:247:
* time and hopefully getting the newer version.
Wouldn't the older version in the other server already be expired/stale when
being served? In that case, this server would fetch the resource and update its
cache, and serve the updated data?
Correct me if I am mistaken.

[zhoresh, 2010-09-30 20:12:16]

On Thu, Sep 30, 2010 at 12:58 PM, <kuntal.loya@gmail.com> wrote:

> Thanks Ziv for making this change.
> I have a couple of doubts though -
>
>
>
>
http://codereview.appspot.com/2298042/diff/8001/java/gadgets/src/main/java/or...
> File
>
>
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/ConcatProxyServlet.java
> (right):
>
>
>
http://codereview.appspot.com/2298042/diff/8001/java/gadgets/src/main/java/or...
>
>
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/ConcatProxyServlet.java:216:
> minTtl = Math.min(minTtl, httpResp.getCacheTtl());
> Just curious, is there a particular reason why the cacheTtl for
> HttpResponse is in ms while for HttpRequest is in seconds?
> The max-age in response headers is supposed to be in seconds.
>

I guess internally ttl is in ms for accuracy and make it easier to work with
System time which is in ms.
But externally the accuracy is not needed.


>
>
>
http://codereview.appspot.com/2298042/diff/8001/java/gadgets/src/main/java/or...
>
> File
>
>
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/ProxyHandler.java
> (right):
>
>
>
http://codereview.appspot.com/2298042/diff/8001/java/gadgets/src/main/java/or...
>
>
>
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/ProxyHandler.java:112:
> proxyUri.translateStatusRefresh(LONG_LIVED_REFRESH, (int)
> (results.getCacheTtl() / 1000)),
> Shouldn't we use the DEFAULT_REFRESH if the response is no-cache or has
> no cache headers?
> Similarly for ConcatProxyHandler
>
I don't think so, basically if the external system indicates not to cache,
then we should pass on that to the user.
For versioned url, we will regenerate the version every time, and only when
content actually change it will generate new version that will bast user
cache.


>
>
>
http://codereview.appspot.com/2298042/diff/8001/java/gadgets/src/main/java/or...
> File
> java/gadgets/src/main/java/org/apache/shindig/gadgets/uri/ProxyUriBase.java
> (right):
>
>
>
http://codereview.appspot.com/2298042/diff/8001/java/gadgets/src/main/java/or...
>
>
java/gadgets/src/main/java/org/apache/shindig/gadgets/uri/ProxyUriBase.java:247:
> * time and hopefully getting the newer version.
> Wouldn't the older version in the other server already be expired/stale
> when being served? In that case, this server would fetch the resource
> and update its cache, and serve the updated data?
> Correct me if I am mistaken.

In theory you are right.
The issue is when the specified caching headers are wrong - basically the
resource was changed even though last retreive indicated it should be cached
for longer time.
In this case the cache will indicate it is not expired yet even though it
was changed on remote server.
I guess the other option is instead to just ignore the cached item and fetch
item again, but that will hurt performance.




>
>
> http://codereview.appspot.com/2298042/
>

[Kuntal Loya, 2010-09-30 20:56:47]

On 2010/09/30 20:12:16, zhoresh wrote:
> On Thu, Sep 30, 2010 at 12:58 PM, <mailto:kuntal.loya@gmail.com> wrote:
> 
> > Thanks Ziv for making this change.
> > I have a couple of doubts though -
> >
> >
> >
> >
>
http://codereview.appspot.com/2298042/diff/8001/java/gadgets/src/main/java/or...
> > File
> >
> >
>
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/ConcatProxyServlet.java
> > (right):
> >
> >
> >
>
http://codereview.appspot.com/2298042/diff/8001/java/gadgets/src/main/java/or...
> >
> >
>
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/ConcatProxyServlet.java:216:
> > minTtl = Math.min(minTtl, httpResp.getCacheTtl());
> > Just curious, is there a particular reason why the cacheTtl for
> > HttpResponse is in ms while for HttpRequest is in seconds?
> > The max-age in response headers is supposed to be in seconds.
> >
> 
> I guess internally ttl is in ms for accuracy and make it easier to work with
> System time which is in ms.
> But externally the accuracy is not needed.
> 
> 
> >
> >
> >
>
http://codereview.appspot.com/2298042/diff/8001/java/gadgets/src/main/java/or...
> >
> > File
> >
> >
>
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/ProxyHandler.java
> > (right):
> >
> >
> >
>
http://codereview.appspot.com/2298042/diff/8001/java/gadgets/src/main/java/or...
> >
> >
> >
>
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/ProxyHandler.java:112:
> > proxyUri.translateStatusRefresh(LONG_LIVED_REFRESH, (int)
> > (results.getCacheTtl() / 1000)),
> > Shouldn't we use the DEFAULT_REFRESH if the response is no-cache or has
> > no cache headers?
> > Similarly for ConcatProxyHandler
> >
> I don't think so, basically if the external system indicates not to cache,
> then we should pass on that to the user.
> For versioned url, we will regenerate the version every time, and only when
> content actually change it will generate new version that will bast user
> cache.
> 
> 
> >
> >
> >
>
http://codereview.appspot.com/2298042/diff/8001/java/gadgets/src/main/java/or...
> > File
> > java/gadgets/src/main/java/org/apache/shindig/gadgets/uri/ProxyUriBase.java
> > (right):
> >
> >
> >
>
http://codereview.appspot.com/2298042/diff/8001/java/gadgets/src/main/java/or...
> >
> >
>
java/gadgets/src/main/java/org/apache/shindig/gadgets/uri/ProxyUriBase.java:247:
> > * time and hopefully getting the newer version.
> > Wouldn't the older version in the other server already be expired/stale
> > when being served? In that case, this server would fetch the resource
> > and update its cache, and serve the updated data?
> > Correct me if I am mistaken.
> 
> In theory you are right.
> The issue is when the specified caching headers are wrong - basically the
> resource was changed even though last retreive indicated it should be cached
> for longer time.
> In this case the cache will indicate it is not expired yet even though it
> was changed on remote server.
> I guess the other option is instead to just ignore the cached item and fetch
> item again, but that will hurt performance.

Okay, that sounds right. We would want to serve the right content as far as
possible even though the resource could have been stale for some users if they
were accessing the page directly.


> 
> 
> 
> 
> >
> >
> > http://codereview.appspot.com/2298042/
> >
>

[Kuntal Loya, 2010-10-01 08:03:30]

http://codereview.appspot.com/2298042/diff/8001/java/gadgets/src/main/java/or...
File
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/ConcatProxyServlet.java
(right):

http://codereview.appspot.com/2298042/diff/8001/java/gadgets/src/main/java/or...
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/ConcatProxyServlet.java:130:
response.setHeader("Content-Type", concatType.getMimeType() + "; charset=UTF8");
The setHeader and setStatus don't really need to be in the try block.

http://codereview.appspot.com/2298042/diff/8001/java/gadgets/src/main/java/or...
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/ConcatProxyServlet.java:205:
if (httpResp != null) {
Is there a possibility of httpResp being null? In that case computation of
minTtle will throw a NPE.

http://codereview.appspot.com/2298042/diff/8001/java/gadgets/src/main/java/or...
File
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/ProxyHandler.java
(right):

http://codereview.appspot.com/2298042/diff/8001/java/gadgets/src/main/java/or...
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/ProxyHandler.java:48:
static final Integer DEFAULT_REFRESH = (60 * 60);                // 1 hour
We can remove DEFAULT_REFRESH if it is not getting used anywhere.

http://codereview.appspot.com/2298042/diff/8001/java/gadgets/src/test/java/or...
File
java/gadgets/src/test/java/org/apache/shindig/gadgets/servlet/ConcatProxyServletTest.java
(right):

http://codereview.appspot.com/2298042/diff/8001/java/gadgets/src/test/java/or...
java/gadgets/src/test/java/org/apache/shindig/gadgets/servlet/ConcatProxyServletTest.java:53:
private static final long TTL3 = 5;
Can you please add a test where all the concat URLs return a 200 and one of them
has -ve or 0 cache ttl.

http://codereview.appspot.com/2298042/diff/8001/java/gadgets/src/test/java/or...
java/gadgets/src/test/java/org/apache/shindig/gadgets/servlet/ConcatProxyServletTest.java:126:
private void runConcat(ExecutorService exec, String result, String tok, long
expires,
add doc for expires as well.

http://codereview.appspot.com/2298042/diff/8001/java/gadgets/src/test/java/or...
java/gadgets/src/test/java/org/apache/shindig/gadgets/servlet/ConcatProxyServletTest.java:231:
assertEquals(400, recorder.getHttpStatusCode());
this response should not have any cache headers.

http://codereview.appspot.com/2298042/diff/8001/java/gadgets/src/test/java/or...
java/gadgets/src/test/java/org/apache/shindig/gadgets/servlet/ConcatProxyServletTest.java:246:
runConcat(sequentialExecutor, results, "_js", TTL1, URL1, URL4);
Shouldn't this have an expiry of -1 seconds (min of 10 and -1)?

http://codereview.appspot.com/2298042/diff/8001/java/gadgets/src/test/java/or...
java/gadgets/src/test/java/org/apache/shindig/gadgets/servlet/ConcatProxyServletTest.java:264:
assertEquals(400, recorder.getHttpStatusCode());
This response will not have a cache header, right?
(Irrespective of the ttl for url4)

http://codereview.appspot.com/2298042/diff/8001/java/gadgets/src/test/java/or...
java/gadgets/src/test/java/org/apache/shindig/gadgets/servlet/ConcatProxyServletTest.java:285:
assertEquals(400, recorder.getHttpStatusCode());
Bad request shoudl again be non cacheable.

http://codereview.appspot.com/2298042/diff/8001/java/gadgets/src/test/java/or...
File
java/gadgets/src/test/java/org/apache/shindig/gadgets/servlet/ProxyHandlerTest.java
(right):

http://codereview.appspot.com/2298042/diff/8001/java/gadgets/src/test/java/or...
java/gadgets/src/test/java/org/apache/shindig/gadgets/servlet/ProxyHandlerTest.java:394:
assertTrue(Math.abs(120 - proxyResp.getCacheTtl() / 1000) < 2);
Could you explain what 2 (and 3 in the next text) are?

[Kuntal Loya, 2010-10-01 10:46:38]

http://codereview.appspot.com/2298042/diff/8001/java/gadgets/src/main/java/or...
File
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/ConcatProxyServlet.java
(right):

http://codereview.appspot.com/2298042/diff/8001/java/gadgets/src/main/java/or...
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/ConcatProxyServlet.java:130:
response.setHeader("Content-Type", concatType.getMimeType() + "; charset=UTF8");
Looks like the content-type header should be set before writing anything to the
response output stream in doFetchConcatResources, else an IllegalStateException
is thrown.

[Kuntal Loya, 2010-10-01 11:12:06]

http://codereview.appspot.com/2298042/diff/8001/java/gadgets/src/main/java/or...
File
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/ConcatProxyServlet.java
(right):

http://codereview.appspot.com/2298042/diff/8001/java/gadgets/src/main/java/or...
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/ConcatProxyServlet.java:130:
response.setHeader("Content-Type", concatType.getMimeType() + "; charset=UTF8");
On 2010/10/01 10:46:38, Kuntal Loya wrote:
> Looks like the content-type header should be set before writing anything to
the
> response output stream in doFetchConcatResources, else an
IllegalStateException
> is thrown.
> 

Moreover, all other headers (cache etc) and the response status should be set
before we start writing to the response output stream. Anything set after that
will not be sent to the user. Therefore, setting the cache headers at the end of
doFetchConcatResources and setting the OK/BAD_REQUEST status here are no-op.

[zhoresh, 2010-10-01 22:16:44]

On Fri, Oct 1, 2010 at 1:03 AM, <kuntal.loya@gmail.com> wrote:

>
>
>
http://codereview.appspot.com/2298042/diff/8001/java/gadgets/src/main/java/or...
> File
>
>
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/ConcatProxyServlet.java
> (right):
>
>
>
http://codereview.appspot.com/2298042/diff/8001/java/gadgets/src/main/java/or...
>
>
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/ConcatProxyServlet.java:130:
> response.setHeader("Content-Type", concatType.getMimeType() + ";
> charset=UTF8");
> The setHeader and setStatus don't really need to be in the try block.
>
Updated as you commented later, the header should be updated before the
stream.


>
>
>
http://codereview.appspot.com/2298042/diff/8001/java/gadgets/src/main/java/or...
>
>
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/ConcatProxyServlet.java:205:
> if (httpResp != null) {
> Is there a possibility of httpResp being null? In that case computation
> of minTtle will throw a NPE.

minTtl is calculated only for non null responses

>
>
>
>
http://codereview.appspot.com/2298042/diff/8001/java/gadgets/src/main/java/or...
> File
>
>
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/ProxyHandler.java
> (right):
>
>
>
http://codereview.appspot.com/2298042/diff/8001/java/gadgets/src/main/java/or...
>
>
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/ProxyHandler.java:48:
> static final Integer DEFAULT_REFRESH = (60 * 60);                // 1
> hour
> We can remove DEFAULT_REFRESH if it is not getting used anywhere.
>
Removed

>
>
>
http://codereview.appspot.com/2298042/diff/8001/java/gadgets/src/test/java/or...
> File
>
>
java/gadgets/src/test/java/org/apache/shindig/gadgets/servlet/ConcatProxyServletTest.java
> (right):
>
>
>
http://codereview.appspot.com/2298042/diff/8001/java/gadgets/src/test/java/or...
>
>
java/gadgets/src/test/java/org/apache/shindig/gadgets/servlet/ConcatProxyServletTest.java:53:
> private static final long TTL3 = 5;
> Can you please add a test where all the concat URLs return a 200 and one
> of them has -ve or 0 cache ttl.
>
Added


>
>
>
http://codereview.appspot.com/2298042/diff/8001/java/gadgets/src/test/java/or...
>
>
java/gadgets/src/test/java/org/apache/shindig/gadgets/servlet/ConcatProxyServletTest.java:126:
> private void runConcat(ExecutorService exec, String result, String tok,
> long expires,
> add doc for expires as well.
>
Added


>
>
>
http://codereview.appspot.com/2298042/diff/8001/java/gadgets/src/test/java/or...
>
>
java/gadgets/src/test/java/org/apache/shindig/gadgets/servlet/ConcatProxyServletTest.java:231:
> assertEquals(400, recorder.getHttpStatusCode());
> this response should not have any cache headers.
>
Actually it will since we set header before failing on stream


>
>
>
http://codereview.appspot.com/2298042/diff/8001/java/gadgets/src/test/java/or...
>
>
java/gadgets/src/test/java/org/apache/shindig/gadgets/servlet/ConcatProxyServletTest.java:246:
> runConcat(sequentialExecutor, results, "_js", TTL1, URL1, URL4);
> Shouldn't this have an expiry of -1 seconds (min of 10 and -1)?
>
There is code specifically for errors that ignore the ttl from it, added
comment

>
>
>
http://codereview.appspot.com/2298042/diff/8001/java/gadgets/src/test/java/or...
>
>
java/gadgets/src/test/java/org/apache/shindig/gadgets/servlet/ConcatProxyServletTest.java:264:
> assertEquals(400, recorder.getHttpStatusCode());
> This response will not have a cache header, right?
> (Irrespective of the ttl for url4)
>
>
>
http://codereview.appspot.com/2298042/diff/8001/java/gadgets/src/test/java/or...
>
>
java/gadgets/src/test/java/org/apache/shindig/gadgets/servlet/ConcatProxyServletTest.java:285:
> assertEquals(400, recorder.getHttpStatusCode());
> Bad request shoudl again be non cacheable.
>

Bad requests are cached to avoid overloading external system


>
>
>
http://codereview.appspot.com/2298042/diff/8001/java/gadgets/src/test/java/or...
> File
>
>
java/gadgets/src/test/java/org/apache/shindig/gadgets/servlet/ProxyHandlerTest.java
> (right):
>
>
>
http://codereview.appspot.com/2298042/diff/8001/java/gadgets/src/test/java/or...
>
>
java/gadgets/src/test/java/org/apache/shindig/gadgets/servlet/ProxyHandlerTest.java:394:
> assertTrue(Math.abs(120 - proxyResp.getCacheTtl() / 1000) < 2);
> Could you explain what 2 (and 3 in the next text) are?

Basically it was to overcome the usage of system time, I fixed it to use
fake time, so there is no need to account to run time anymore


>
>
> http://codereview.appspot.com/2298042/
>

[zhoresh, 2010-10-07 00:51:02]

Delayed updated patch

[Kuntal Loya, 2010-10-07 11:30:41]

http://codereview.appspot.com/2298042/diff/24001/java/gadgets/src/main/java/o...
File
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/ConcatProxyServlet.java
(right):

http://codereview.appspot.com/2298042/diff/24001/java/gadgets/src/main/java/o...
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/ConcatProxyServlet.java:214:
cos.output(futureTask.one, httpResp);
We have started streaming the output here, however the cache-header as well as
the response status are not set yet. Response status will default to 200 though,
while the cache-control will be private, max-age=0.

http://codereview.appspot.com/2298042/diff/24001/java/gadgets/src/test/java/o...
File
java/gadgets/src/test/java/org/apache/shindig/gadgets/servlet/ConcatProxyServletTest.java
(right):

http://codereview.appspot.com/2298042/diff/24001/java/gadgets/src/test/java/o...
java/gadgets/src/test/java/org/apache/shindig/gadgets/servlet/ConcatProxyServletTest.java:192:
+ "HTML_PARSE_ERROR concat(http://example.org/4.js) null";
A generic concat doubt -
When there is an error while concatenating a url, the response we send is
something like - 
"HTML_PARSE_ERROR concat(http://example.org/4.js) null"  not as a JS comment,
but rather a JS statement. In that case, when the browser will try to execute
it, and will throw a JS error.
This should not be the expected behavior.

[zhoresh, 2010-10-07 17:24:20]

Minimize change, remove concat fix for now

[zhoresh, 2010-10-07 17:25:54]

Lets minimize this change to fix only proxy.
Concat will be a separate change.

[johnfargo, 2010-10-07 19:51:17]

Excellent change. One nit on the config.

http://codereview.appspot.com/2298042/diff/31001/java/gadgets/src/main/java/o...
File
java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ContentRewriterFeature.java
(right):

http://codereview.appspot.com/2298042/diff/31001/java/gadgets/src/main/java/o...
java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ContentRewriterFeature.java:267:
expiresVal = (expiresVal < 0 || overrideVal < expiresVal) ? overrideVal :
expiresVal;
I don't remember the original rationale for this, but this logic prevents the
overrideVal from extending the expiration time. That doesn't seem quite right --
if the developer wants a higher forced cache rate for rewritten resources I'd
say that's A-OK. Thoughts?

http://codereview.appspot.com/2298042/diff/31001/java/gadgets/src/main/java/o...
java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ContentRewriterFeature.java:271:
expiresVal = EXPIRES_DEFAULT;
while we're at it, we might consider renaming EXPIRES_DEFAULT to EXPIRES_HTTP.
We could later use this const to make the code's logic more self-documenting vis
a vis the value "-1".

[johnfargo, 2010-10-07 19:57:00]

Clarification, Spec:
http://wiki.opensocial.org/index.php?title=Content_Rewriter_Feature

The spec indicates that the "expires" parameter, whether provided as a default
or by the gadget developer, is supposed to indicate the minimum HTTP cache time
for served requests, not the maximum or fixed value, as currently implemented:
"The duration in seconds to force as minimum HTTP cache time for content
fetched..."

Thread
http://www.mail-archive.com/shindig-dev@incubator.apache.org/msg11804.html
discusses an inverse rationale, that "As the host of the gadget server, I have
decided what URLs
and types and ttls are suitable for my installation. I do not want a
gadget author to be able to increase any of those values..."

So despite the conflicting opinion, I retract my suggestion, since this appears
to be an issue with existing ambiguity. No need to block the CL while we debate
:)

On 2010/10/07 19:51:17, johnfargo wrote:
> Excellent change. One nit on the config.
> 
>
http://codereview.appspot.com/2298042/diff/31001/java/gadgets/src/main/java/o...
> File
>
java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ContentRewriterFeature.java
> (right):
> 
>
http://codereview.appspot.com/2298042/diff/31001/java/gadgets/src/main/java/o...
>
java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ContentRewriterFeature.java:267:
> expiresVal = (expiresVal < 0 || overrideVal < expiresVal) ? overrideVal :
> expiresVal;
> I don't remember the original rationale for this, but this logic prevents the
> overrideVal from extending the expiration time. That doesn't seem quite right
--
> if the developer wants a higher forced cache rate for rewritten resources I'd
> say that's A-OK. Thoughts?
> 
>
http://codereview.appspot.com/2298042/diff/31001/java/gadgets/src/main/java/o...
>
java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ContentRewriterFeature.java:271:
> expiresVal = EXPIRES_DEFAULT;
> while we're at it, we might consider renaming EXPIRES_DEFAULT to EXPIRES_HTTP.
> We could later use this const to make the code's logic more self-documenting
vis
> a vis the value "-1".

[zhoresh, 2010-10-07 22:26:23]

John,
Thanks for tracking down the spec.
It is interesting that the spec specify a day as a minimum value, which
might be the reason for the confusion.
But this specific if statement is not for setting resource expire time, but
more to choose what minimum time to use.
Basically it compare the default minimum time (spec indicate 1 day) to what
the user specify in the gadget spec,
and will take the minimum of the two (so user cannot specify a minimum cache
time to be less then a day).
But the default minimum can actually be configured, and can be -1, which in
this context -1 (or HTML), indicate no minimum.
My change allow gadget spec to override the special -1 value.

Other then that I agree with you, I rather see the default minimum be
smaller value, and allow spec to make it larger not smaller.
Operation wise I actually think that the default minimum should be -1

I can take that change out as well, so at least I can submit the proxy fix.

-Ziv



On Thu, Oct 7, 2010 at 12:57 PM, <johnfargo@gmail.com> wrote:

> Clarification, Spec:
> http://wiki.opensocial.org/index.php?title=Content_Rewriter_Feature
>
> The spec indicates that the "expires" parameter, whether provided as a
> default or by the gadget developer, is supposed to indicate the minimum
> HTTP cache time for served requests, not the maximum or fixed value, as
> currently implemented: "The duration in seconds to force as minimum HTTP
> cache time for content fetched..."
>
> Thread
> http://www.mail-archive.com/shindig-dev@incubator.apache.org/msg11804.html
> discusses an inverse rationale, that "As the host of the gadget server,
> I have decided what URLs
> and types and ttls are suitable for my installation. I do not want a
> gadget author to be able to increase any of those values..."
>
> So despite the conflicting opinion, I retract my suggestion, since this
> appears to be an issue with existing ambiguity. No need to block the CL
> while we debate :)
>
>
> On 2010/10/07 19:51:17, johnfargo wrote:
>
>> Excellent change. One nit on the config.
>>
>
>
>
>
http://codereview.appspot.com/2298042/diff/31001/java/gadgets/src/main/java/o...
>
>> File
>>
>
>
>
java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ContentRewriterFeature.java
>
>> (right):
>>
>
>
>
>
http://codereview.appspot.com/2298042/diff/31001/java/gadgets/src/main/java/o...
>
>
>
java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ContentRewriterFeature.java:267:
>
>> expiresVal = (expiresVal < 0 || overrideVal < expiresVal) ?
>>
> overrideVal :
>
>> expiresVal;
>> I don't remember the original rationale for this, but this logic
>>
> prevents the
>
>> overrideVal from extending the expiration time. That doesn't seem
>>
> quite right --
>
>> if the developer wants a higher forced cache rate for rewritten
>>
> resources I'd
>
>> say that's A-OK. Thoughts?
>>
>
>
>
>
http://codereview.appspot.com/2298042/diff/31001/java/gadgets/src/main/java/o...
>
>
>
java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ContentRewriterFeature.java:271:
>
>> expiresVal = EXPIRES_DEFAULT;
>> while we're at it, we might consider renaming EXPIRES_DEFAULT to
>>
> EXPIRES_HTTP.
>
>> We could later use this const to make the code's logic more
>>
> self-documenting vis
>
>> a vis the value "-1".
>>
>
>
>
> http://codereview.appspot.com/2298042/
>

[zhoresh, 2010-10-07 22:40:14]

Update name to make it more clear as John suggested

[johnfargo, 2010-10-07 22:41:28]

LGTM

On 2010/10/07 22:40:14, zhoresh wrote:
> Update name to make it more clear as John suggested

[zhoresh, 2010-10-07 23:21:08]

Thanks, committed as r1005661


On Thu, Oct 7, 2010 at 3:41 PM, <johnfargo@gmail.com> wrote:

> LGTM
>
>
> On 2010/10/07 22:40:14, zhoresh wrote:
>
>> Update name to make it more clear as John suggested
>>
>
>
>
> http://codereview.appspot.com/2298042/
>

[gagan.goku, 2010-10-09 14:01:37]

Please close this issue as it has been submitted.