Issue 3660043: Cache Ttl of concat resource should be minimum(cache ttl of included resources) when unversioned (Closed)
|
Created:
15 years, 2 months ago by pulkitgoyal2000 Modified:
14 years, 11 months ago Base URL:
http://svn.apache.org/repos/asf/shindig/trunk/ Visibility:
Public. |
DescriptionCache Ttl value is set to 1 hr for concat resource when it is
unversioned. This value should set to minimum of all
individual resources' cache ttl value present in concat resource
Patch Set 1 #Patch Set 2 : Correcting Old Chuck Error in ConcatProxyServlet.java #
Total comments: 16
Patch Set 3 : Updated with Satya's Comment #
Total comments: 5
Patch Set 4 : Simpler approach for setting MinCache Ttl. #
Total comments: 1
Patch Set 5 : Previous patch fails in few scenarios so reverting back to earlier patch #
Total comments: 7
Patch Set 6 : Addressing Satya's Comment #
Total comments: 8
Patch Set 7 : Updated Patch with new Test #Patch Set 8 : Updated Patch with new Test #
Total comments: 2
Patch Set 9 : Addressing Comment #Patch Set 10 : Uploading New Patch to remove mismatch error #
Total comments: 45
Patch Set 11 : Addressing Comments #
Total comments: 4
Patch Set 12 : Addressing Nikhil's Comment #
Total comments: 6
Patch Set 13 : Addressing comments #Patch Set 14 : Remove some extra lines #
Total comments: 2
Patch Set 15 : Addressing Fargo's Comment #Patch Set 16 : Addressing Comments #
Total comments: 2
Patch Set 17 : Addressing Gagan's Comment #
MessagesTotal messages: 31
Sign in to reply to this message.
Correcting Old Chuck Error in ConcatProxyServlet.java
Sign in to reply to this message.
http://codereview.appspot.com/3660043/diff/11001/java/gadgets/src/main/java/o... File java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/ConcatProxyServlet.java (right): http://codereview.appspot.com/3660043/diff/11001/java/gadgets/src/main/java/o... java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/ConcatProxyServlet.java:238: ConcatOutputStream cos = null; Can you check if streaming the response is giving any benefits here. I'd recommend you return the computed concat response via the doGet method like ProxyServlet.
Sign in to reply to this message.
some minor comments http://codereview.appspot.com/3660043/diff/11001/java/gadgets/src/main/java/o... File java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/ConcatProxyServlet.java (right): http://codereview.appspot.com/3660043/diff/11001/java/gadgets/src/main/java/o... java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/ConcatProxyServlet.java:162: /** Add description to the function http://codereview.appspot.com/3660043/diff/11001/java/gadgets/src/main/java/o... java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/ConcatProxyServlet.java:206: minCacheTtl = Math.min(minCacheTtl,(int) (httpResp.getCacheTtl() / 1000) ); default for httpResp.getCacheTtl() can return -1, so u need to distinguish between error -1 with this. http://codereview.appspot.com/3660043/diff/11001/java/gadgets/src/main/java/o... java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/ConcatProxyServlet.java:209: httpResp = contentRewriterRegistry.rewriteHttpResponse(requestCxt.getHttpReq(), indent +4 http://codereview.appspot.com/3660043/diff/11001/java/gadgets/src/main/java/o... java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/ConcatProxyServlet.java:224: return minCacheTtl; +2 http://codereview.appspot.com/3660043/diff/11001/java/gadgets/src/test/java/o... File java/gadgets/src/test/java/org/apache/shindig/gadgets/servlet/ConcatProxyServletTest.java (right): http://codereview.appspot.com/3660043/diff/11001/java/gadgets/src/test/java/o... java/gadgets/src/test/java/org/apache/shindig/gadgets/servlet/ConcatProxyServletTest.java:82: private void expectGetAndSetCacheTtl(Uri url, Integer cacheTtl) throws Exception { Do you mind moving this function definition, after testMinimumCacheTtl() http://codereview.appspot.com/3660043/diff/11001/java/gadgets/src/test/java/o... java/gadgets/src/test/java/org/apache/shindig/gadgets/servlet/ConcatProxyServletTest.java:85: expect(pipeline.execute(req)).andReturn(resp).anyTimes(); you don't need .anyTimes() http://codereview.appspot.com/3660043/diff/11001/java/gadgets/src/test/java/o... java/gadgets/src/test/java/org/apache/shindig/gadgets/servlet/ConcatProxyServletTest.java:307: String cacheValue = cacheControl.substring(cacheControl.indexOf('=')+1); cacheControl.indexOf('=') + 1
Sign in to reply to this message.
http://codereview.appspot.com/3660043/diff/11001/java/gadgets/src/main/java/o... File java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/ConcatProxyServlet.java (right): http://codereview.appspot.com/3660043/diff/11001/java/gadgets/src/main/java/o... java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/ConcatProxyServlet.java:162: /** On 2010/12/16 12:27:18, satya3656 wrote: > Add description to the function Done. http://codereview.appspot.com/3660043/diff/11001/java/gadgets/src/main/java/o... java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/ConcatProxyServlet.java:206: minCacheTtl = Math.min(minCacheTtl,(int) (httpResp.getCacheTtl() / 1000) ); On 2010/12/16 12:27:18, satya3656 wrote: > default for httpResp.getCacheTtl() can return -1, so u need to distinguish > between error -1 with this. Done. http://codereview.appspot.com/3660043/diff/11001/java/gadgets/src/main/java/o... java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/ConcatProxyServlet.java:209: httpResp = contentRewriterRegistry.rewriteHttpResponse(requestCxt.getHttpReq(), On 2010/12/16 12:27:18, satya3656 wrote: > indent +4 Done. http://codereview.appspot.com/3660043/diff/11001/java/gadgets/src/main/java/o... java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/ConcatProxyServlet.java:224: return minCacheTtl; On 2010/12/16 12:27:18, satya3656 wrote: > +2 Done. http://codereview.appspot.com/3660043/diff/11001/java/gadgets/src/main/java/o... java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/ConcatProxyServlet.java:238: ConcatOutputStream cos = null; On 2010/12/16 10:04:30, Kuntal Loya wrote: > Can you check if streaming the response is giving any benefits here. > I'd recommend you return the computed concat response via the doGet method like > ProxyServlet. We will track this issue with separate bug. http://codereview.appspot.com/3660043/diff/11001/java/gadgets/src/test/java/o... File java/gadgets/src/test/java/org/apache/shindig/gadgets/servlet/ConcatProxyServletTest.java (right): http://codereview.appspot.com/3660043/diff/11001/java/gadgets/src/test/java/o... java/gadgets/src/test/java/org/apache/shindig/gadgets/servlet/ConcatProxyServletTest.java:82: private void expectGetAndSetCacheTtl(Uri url, Integer cacheTtl) throws Exception { On 2010/12/16 12:27:18, satya3656 wrote: > Do you mind moving this function definition, after testMinimumCacheTtl() Done. http://codereview.appspot.com/3660043/diff/11001/java/gadgets/src/test/java/o... java/gadgets/src/test/java/org/apache/shindig/gadgets/servlet/ConcatProxyServletTest.java:85: expect(pipeline.execute(req)).andReturn(resp).anyTimes(); On 2010/12/16 12:27:18, satya3656 wrote: > you don't need .anyTimes() > Done. http://codereview.appspot.com/3660043/diff/11001/java/gadgets/src/test/java/o... java/gadgets/src/test/java/org/apache/shindig/gadgets/servlet/ConcatProxyServletTest.java:307: String cacheValue = cacheControl.substring(cacheControl.indexOf('=')+1); On 2010/12/16 12:27:18, satya3656 wrote: > cacheControl.indexOf('=') + 1 Done.
Sign in to reply to this message.
Hi Pulkit, After bit of discussion with gagan, the current approach will make the response not stream-able. So instead let us do this, Check if all the resources are in cache, set the ttl accordingly, else set set it to 0. What do you think about this? http://codereview.appspot.com/3660043/diff/21001/java/gadgets/src/main/java/o... File java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/ConcatProxyServlet.java (right): http://codereview.appspot.com/3660043/diff/21001/java/gadgets/src/main/java/o... java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/ConcatProxyServlet.java:167: /** Returns the minimum Cache Ttl value among all the provided responses. New line after description http://codereview.appspot.com/3660043/diff/21001/java/gadgets/src/main/java/o... java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/ConcatProxyServlet.java:172: * null means Bad Request. Will fit in previous line http://codereview.appspot.com/3660043/diff/21001/java/gadgets/src/main/java/o... java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/ConcatProxyServlet.java:233: /** Description for this function
Sign in to reply to this message.
LGTM Can you once check if everything is working as expected on your local server. http://codereview.appspot.com/3660043/diff/33001/java/gadgets/src/main/java/o... File java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/ConcatProxyServlet.java (right): http://codereview.appspot.com/3660043/diff/33001/java/gadgets/src/main/java/o... java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/ConcatProxyServlet.java:246: if (cos.outputError(null, gex)) { Why is this check needed? try block has nothing to do with cos
Sign in to reply to this message.
Previous patch fails in few scenarios so reverting back to earlier patch
Sign in to reply to this message.
There is some chunck mismatch for Test file. And some minor comments. http://codereview.appspot.com/3660043/diff/42001/java/gadgets/src/main/java/o... File java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/ConcatProxyServlet.java (right): http://codereview.appspot.com/3660043/diff/42001/java/gadgets/src/main/java/o... java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/ConcatProxyServlet.java:129: Long minCacheTtl = getMinCacheTtlFetchResources(response, concatUri, responsesUriMap); I prefer long to Long http://codereview.appspot.com/3660043/diff/42001/java/gadgets/src/main/java/o... java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/ConcatProxyServlet.java:139: remove this line http://codereview.appspot.com/3660043/diff/42001/java/gadgets/src/main/java/o... java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/ConcatProxyServlet.java:233: /** Output concat response to client. /** * Output concat response to client.
Sign in to reply to this message.
http://codereview.appspot.com/3660043/diff/21001/java/gadgets/src/main/java/o... File java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/ConcatProxyServlet.java (right): http://codereview.appspot.com/3660043/diff/21001/java/gadgets/src/main/java/o... java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/ConcatProxyServlet.java:167: /** Returns the minimum Cache Ttl value among all the provided responses. On 2010/12/28 10:03:49, satya3656 wrote: > New line after description Done. http://codereview.appspot.com/3660043/diff/21001/java/gadgets/src/main/java/o... java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/ConcatProxyServlet.java:172: * null means Bad Request. On 2010/12/28 10:03:49, satya3656 wrote: > Will fit in previous line Done. http://codereview.appspot.com/3660043/diff/42001/java/gadgets/src/main/java/o... java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/ConcatProxyServlet.java:129: Long minCacheTtl = getMinCacheTtlFetchResources(response, concatUri, responsesUriMap); On 2011/01/17 16:45:21, satya3656 wrote: > I prefer long to Long It return null to check whether response is valid or not. http://codereview.appspot.com/3660043/diff/42001/java/gadgets/src/main/java/o... java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/ConcatProxyServlet.java:139: On 2011/01/17 16:45:21, satya3656 wrote: > remove this line Done. http://codereview.appspot.com/3660043/diff/42001/java/gadgets/src/main/java/o... java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/ConcatProxyServlet.java:233: /** On 2011/01/17 16:45:21, satya3656 wrote: > /** > * Output concat response to client. Done.
Sign in to reply to this message.
http://codereview.appspot.com/3660043/diff/42001/java/gadgets/src/main/java/o... File java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/ConcatProxyServlet.java (right): http://codereview.appspot.com/3660043/diff/42001/java/gadgets/src/main/java/o... java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/ConcatProxyServlet.java:129: Long minCacheTtl = getMinCacheTtlFetchResources(response, concatUri, responsesUriMap); On 2011/01/19 07:35:08, pulkitgoyal2000 wrote: > On 2011/01/17 16:45:21, satya3656 wrote: > > I prefer long to Long > It return null to check whether response is valid or not. You can return -2 or something like that as well http://codereview.appspot.com/3660043/diff/52001/java/gadgets/src/main/java/o... java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/ConcatProxyServlet.java:128: HashMap<Uri, Object> responsesUriMap = new HashMap<Uri, Object>(); can you name it uriReponseMap which is more appropriate http://codereview.appspot.com/3660043/diff/52001/java/gadgets/src/test/java/o... File java/gadgets/src/test/java/org/apache/shindig/gadgets/servlet/ConcatProxyServletTest.java (right): http://codereview.appspot.com/3660043/diff/52001/java/gadgets/src/test/java/o... java/gadgets/src/test/java/org/apache/shindig/gadgets/servlet/ConcatProxyServletTest.java:302: String cacheControl = recorder.getHeader("Cache-Control"); Can you add one more test, and check if indeed we are getting cache ttl as zero if one of the input uri's don't have the cache headers http://codereview.appspot.com/3660043/diff/52001/java/gadgets/src/test/java/o... java/gadgets/src/test/java/org/apache/shindig/gadgets/servlet/ConcatProxyServletTest.java:326: Uri uri = new UriBuilder(request).toUri(); Please move these two lines before replay() or after verify() http://codereview.appspot.com/3660043/diff/52001/java/gadgets/src/test/java/o... java/gadgets/src/test/java/org/apache/shindig/gadgets/servlet/ConcatProxyServletTest.java:337: public List<ConcatData> make( fits in previous line
Sign in to reply to this message.
http://codereview.appspot.com/3660043/diff/52001/java/gadgets/src/main/java/o... File java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/ConcatProxyServlet.java (right): http://codereview.appspot.com/3660043/diff/52001/java/gadgets/src/main/java/o... java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/ConcatProxyServlet.java:128: HashMap<Uri, Object> responsesUriMap = new HashMap<Uri, Object>(); On 2011/01/20 07:11:23, satya3656 wrote: > can you name it uriReponseMap which is more appropriate Done. http://codereview.appspot.com/3660043/diff/52001/java/gadgets/src/test/java/o... File java/gadgets/src/test/java/org/apache/shindig/gadgets/servlet/ConcatProxyServletTest.java (right): http://codereview.appspot.com/3660043/diff/52001/java/gadgets/src/test/java/o... java/gadgets/src/test/java/org/apache/shindig/gadgets/servlet/ConcatProxyServletTest.java:302: String cacheControl = recorder.getHeader("Cache-Control"); On 2011/01/20 07:11:23, satya3656 wrote: > Can you add one more test, and check if indeed we are getting cache ttl as zero > if one of the input uri's don't have the cache headers Done. http://codereview.appspot.com/3660043/diff/52001/java/gadgets/src/test/java/o... java/gadgets/src/test/java/org/apache/shindig/gadgets/servlet/ConcatProxyServletTest.java:326: Uri uri = new UriBuilder(request).toUri(); On 2011/01/20 07:11:23, satya3656 wrote: > Please move these two lines before replay() or after verify() Not Changed. Otherwise request variable is not set with parameters like scheme etc & throws error. http://codereview.appspot.com/3660043/diff/52001/java/gadgets/src/test/java/o... java/gadgets/src/test/java/org/apache/shindig/gadgets/servlet/ConcatProxyServletTest.java:337: public List<ConcatData> make( On 2011/01/20 07:11:23, satya3656 wrote: > fits in previous line Done.
Sign in to reply to this message.
LGTM http://codereview.appspot.com/3660043/diff/64001/java/gadgets/src/main/java/o... File java/gadgets/src/main/java/org/apache/shindig/gadgets/http/HttpResponse.java (right): http://codereview.appspot.com/3660043/diff/64001/java/gadgets/src/main/java/o... java/gadgets/src/main/java/org/apache/shindig/gadgets/http/HttpResponse.java:478: public static long getDefaultTtl() { Why can't you change defaultTtl to public static rather changing this?
Sign in to reply to this message.
http://codereview.appspot.com/3660043/diff/64001/java/gadgets/src/main/java/o... File java/gadgets/src/main/java/org/apache/shindig/gadgets/http/HttpResponse.java (right): http://codereview.appspot.com/3660043/diff/64001/java/gadgets/src/main/java/o... java/gadgets/src/main/java/org/apache/shindig/gadgets/http/HttpResponse.java:478: public static long getDefaultTtl() { On 2011/01/26 07:56:49, satya3656 wrote: > Why can't you change defaultTtl to public static rather changing this? Done.
Sign in to reply to this message.
Reinitiating code review of this cl.
Sign in to reply to this message.
Small nits. http://codereview.appspot.com/3660043/diff/92001/java/gadgets/src/main/java/o... File java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/ConcatProxyServlet.java (right): http://codereview.appspot.com/3660043/diff/92001/java/gadgets/src/main/java/o... java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/ConcatProxyServlet.java:163: * Returns the minimum Cache Ttl value among all the provided responses. Cache Ttl -> cache ttl http://codereview.appspot.com/3660043/diff/92001/java/gadgets/src/main/java/o... java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/ConcatProxyServlet.java:234: * @param responsesUriMap HttpResponse Map based on the last call. map containing responses for the fetched urls. http://codereview.appspot.com/3660043/diff/92001/java/gadgets/src/main/java/o... java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/ConcatProxyServlet.java:240: HashMap<Uri, Object> responsesUriMap) throws IOException { uriResourceMap? http://codereview.appspot.com/3660043/diff/92001/java/gadgets/src/main/java/o... java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/ConcatProxyServlet.java:264: if(responseObj.getClass().getName().equalsIgnoreCase(GadgetException.class.getName())) { Do you really really want to return false and send error if one resource fetch failed? http://codereview.appspot.com/3660043/diff/92001/java/gadgets/src/test/java/o... File java/gadgets/src/test/java/org/apache/shindig/gadgets/servlet/ConcatProxyServletTest.java (right): http://codereview.appspot.com/3660043/diff/92001/java/gadgets/src/test/java/o... java/gadgets/src/test/java/org/apache/shindig/gadgets/servlet/ConcatProxyServletTest.java:324: assertEquals((int) (HttpResponse.defaultTtl / 1000), Integer.decode(cacheValue), 10); can you add a comment on why /1000 is needed here.
Sign in to reply to this message.
http://codereview.appspot.com/3660043/diff/92001/java/gadgets/src/main/java/o... File java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/ConcatProxyServlet.java (right): http://codereview.appspot.com/3660043/diff/92001/java/gadgets/src/main/java/o... java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/ConcatProxyServlet.java:56: import java.util.HashMap; order alphabetically http://codereview.appspot.com/3660043/diff/92001/java/gadgets/src/main/java/o... java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/ConcatProxyServlet.java:129: return; It looks like we are slightly changing the behavior here as compared to before since we don't add the content-type and content-disposition headers? http://codereview.appspot.com/3660043/diff/92001/java/gadgets/src/main/java/o... java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/ConcatProxyServlet.java:135: Also..I'd prefer if we do this computation closer to where we are actually using minCacheTtl http://codereview.appspot.com/3660043/diff/92001/java/gadgets/src/main/java/o... java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/ConcatProxyServlet.java:163: * Returns the minimum Cache Ttl value among all the provided responses. please update the description to mention everything we do in this function, including fetching of resources and filling of uriResponseMap http://codereview.appspot.com/3660043/diff/92001/java/gadgets/src/main/java/o... java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/ConcatProxyServlet.java:167: * @param uriResponseMap HttpResponse Map based on Uri. nit: its a map from uri to object which is generally HttpResponse and could be GadgetException sometimes... http://codereview.appspot.com/3660043/diff/92001/java/gadgets/src/main/java/o... java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/ConcatProxyServlet.java:168: * @return minimum CacheTtl value among all the HttpResponses null means Bad Request. CacheTtl -> cache ttl http://codereview.appspot.com/3660043/diff/92001/java/gadgets/src/main/java/o... java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/ConcatProxyServlet.java:169: * @throws IOException nit : we actually throw GadgetException http://codereview.appspot.com/3660043/diff/92001/java/gadgets/src/main/java/o... java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/ConcatProxyServlet.java:171: private Long getMinCacheTtlFetchResources(HttpServletResponse response, getMinCacheTtlAndFetchResources http://codereview.appspot.com/3660043/diff/92001/java/gadgets/src/main/java/o... java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/ConcatProxyServlet.java:177: for (Uri resourceUri : concatUri.getBatch()) { indentation seems off here. http://codereview.appspot.com/3660043/diff/92001/java/gadgets/src/main/java/o... java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/ConcatProxyServlet.java:182: if (ge.getHttpStatusCode() == HttpResponse.SC_INTERNAL_SERVER_ERROR) { i didn't understand why are we doing this? http://codereview.appspot.com/3660043/diff/92001/java/gadgets/src/main/java/o... java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/ConcatProxyServlet.java:217: } could you fix the indentation here... http://codereview.appspot.com/3660043/diff/92001/java/gadgets/src/main/java/o... java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/ConcatProxyServlet.java:226: return minCacheTtl; There seems to be one code path where we will return Long.MAX_VALUE, if we repeatedly catch GadgetException and do the uriResourceMap.put I'm not sure if it actually matters...just wanted to check that you have considered it..and also that there are no other such paths... http://codereview.appspot.com/3660043/diff/92001/java/gadgets/src/main/java/o... java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/ConcatProxyServlet.java:265: if (cos.outputError(resourceUri, (GadgetException) responseObj)) { could you combine the two ifs into one? http://codereview.appspot.com/3660043/diff/92001/java/gadgets/src/main/java/o... java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/ConcatProxyServlet.java:285: private static String formatHttpError(int status, String errorMessage, Uri uri) { nit: add the space back http://codereview.appspot.com/3660043/diff/92001/java/gadgets/src/test/java/o... File java/gadgets/src/test/java/org/apache/shindig/gadgets/servlet/ConcatProxyServletTest.java (right): http://codereview.appspot.com/3660043/diff/92001/java/gadgets/src/test/java/o... java/gadgets/src/test/java/org/apache/shindig/gadgets/servlet/ConcatProxyServletTest.java:27: import org.apache.shindig.common.servlet.HttpServletResponseRecorder; nit: order http://codereview.appspot.com/3660043/diff/92001/java/gadgets/src/test/java/o... java/gadgets/src/test/java/org/apache/shindig/gadgets/servlet/ConcatProxyServletTest.java:300: don't both the tests need a replay()? http://codereview.appspot.com/3660043/diff/92001/java/gadgets/src/test/java/o... java/gadgets/src/test/java/org/apache/shindig/gadgets/servlet/ConcatProxyServletTest.java:304: String cacheControl = recorder.getHeader("Cache-Control"); use HttpResponse.getCacheControlMaxAge instead? http://codereview.appspot.com/3660043/diff/92001/java/gadgets/src/test/java/o... java/gadgets/src/test/java/org/apache/shindig/gadgets/servlet/ConcatProxyServletTest.java:344: nit: better not to touch stuff not related to the change
Sign in to reply to this message.
http://codereview.appspot.com/3660043/diff/92001/java/gadgets/src/main/java/o... File java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/ConcatProxyServlet.java (right): http://codereview.appspot.com/3660043/diff/92001/java/gadgets/src/main/java/o... java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/ConcatProxyServlet.java:56: import java.util.HashMap; On 2011/03/22 15:16:03, nikhilmadan23 wrote: > order alphabetically Done. http://codereview.appspot.com/3660043/diff/92001/java/gadgets/src/main/java/o... java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/ConcatProxyServlet.java:129: return; On 2011/03/22 15:16:03, nikhilmadan23 wrote: > It looks like we are slightly changing the behavior here as compared to before > since we don't add the content-type and content-disposition headers? Does it matter when response is 400? http://codereview.appspot.com/3660043/diff/92001/java/gadgets/src/main/java/o... java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/ConcatProxyServlet.java:135: On 2011/03/22 15:16:03, nikhilmadan23 wrote: > Also..I'd prefer if we do this computation closer to where we are actually using > minCacheTtl Moved inside try catch block. http://codereview.appspot.com/3660043/diff/92001/java/gadgets/src/main/java/o... java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/ConcatProxyServlet.java:163: * Returns the minimum Cache Ttl value among all the provided responses. On 2011/03/22 14:24:28, satya3656 wrote: > Cache Ttl -> cache ttl Done. http://codereview.appspot.com/3660043/diff/92001/java/gadgets/src/main/java/o... java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/ConcatProxyServlet.java:163: * Returns the minimum Cache Ttl value among all the provided responses. On 2011/03/22 15:16:03, nikhilmadan23 wrote: > please update the description to mention everything we do in this function, > including fetching of resources and filling of uriResponseMap Done. http://codereview.appspot.com/3660043/diff/92001/java/gadgets/src/main/java/o... java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/ConcatProxyServlet.java:167: * @param uriResponseMap HttpResponse Map based on Uri. On 2011/03/22 15:16:03, nikhilmadan23 wrote: > nit: its a map from uri to object which is generally HttpResponse and could be > GadgetException sometimes... Done. http://codereview.appspot.com/3660043/diff/92001/java/gadgets/src/main/java/o... java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/ConcatProxyServlet.java:168: * @return minimum CacheTtl value among all the HttpResponses null means Bad Request. On 2011/03/22 15:16:03, nikhilmadan23 wrote: > CacheTtl -> cache ttl Done. http://codereview.appspot.com/3660043/diff/92001/java/gadgets/src/main/java/o... java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/ConcatProxyServlet.java:169: * @throws IOException On 2011/03/22 15:16:03, nikhilmadan23 wrote: > nit : we actually throw GadgetException response.getOutputStream() throws IOException. http://codereview.appspot.com/3660043/diff/92001/java/gadgets/src/main/java/o... java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/ConcatProxyServlet.java:171: private Long getMinCacheTtlFetchResources(HttpServletResponse response, On 2011/03/22 15:16:03, nikhilmadan23 wrote: > getMinCacheTtlAndFetchResources Done. http://codereview.appspot.com/3660043/diff/92001/java/gadgets/src/main/java/o... java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/ConcatProxyServlet.java:177: for (Uri resourceUri : concatUri.getBatch()) { On 2011/03/22 15:16:03, nikhilmadan23 wrote: > indentation seems off here. Done. http://codereview.appspot.com/3660043/diff/92001/java/gadgets/src/main/java/o... java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/ConcatProxyServlet.java:182: if (ge.getHttpStatusCode() == HttpResponse.SC_INTERNAL_SERVER_ERROR) { On 2011/03/22 15:16:03, nikhilmadan23 wrote: > i didn't understand why are we doing this? I too dont have much idea..I tried to be same as it was earlier. But the reason may be not to continue unnecessarily if HTTP server is itself down. http://codereview.appspot.com/3660043/diff/92001/java/gadgets/src/main/java/o... java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/ConcatProxyServlet.java:217: } On 2011/03/22 15:16:03, nikhilmadan23 wrote: > could you fix the indentation here... Done. http://codereview.appspot.com/3660043/diff/92001/java/gadgets/src/main/java/o... java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/ConcatProxyServlet.java:226: return minCacheTtl; On 2011/03/22 15:16:03, nikhilmadan23 wrote: > There seems to be one code path where we will return Long.MAX_VALUE, if we > repeatedly catch GadgetException and do the uriResourceMap.put > I'm not sure if it actually matters...just wanted to check that you have > considered it..and also that there are no other such paths... As discussed offline, if above case is encountered, we set it to Default value. http://codereview.appspot.com/3660043/diff/92001/java/gadgets/src/main/java/o... java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/ConcatProxyServlet.java:234: * @param responsesUriMap HttpResponse Map based on the last call. On 2011/03/22 14:24:28, satya3656 wrote: > map containing responses for the fetched urls. Done. http://codereview.appspot.com/3660043/diff/92001/java/gadgets/src/main/java/o... java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/ConcatProxyServlet.java:240: HashMap<Uri, Object> responsesUriMap) throws IOException { On 2011/03/22 14:24:28, satya3656 wrote: > uriResourceMap? Changed to uriResponsesMap http://codereview.appspot.com/3660043/diff/92001/java/gadgets/src/main/java/o... java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/ConcatProxyServlet.java:264: if(responseObj.getClass().getName().equalsIgnoreCase(GadgetException.class.getName())) { On 2011/03/22 14:24:28, satya3656 wrote: > Do you really really want to return false and send error if one resource fetch > failed? Only in the case if it is SC_INTERNAL_SERVER_ERROR. http://codereview.appspot.com/3660043/diff/92001/java/gadgets/src/main/java/o... java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/ConcatProxyServlet.java:265: if (cos.outputError(resourceUri, (GadgetException) responseObj)) { On 2011/03/22 15:16:03, nikhilmadan23 wrote: > could you combine the two ifs into one? No.. because there is else on the first if. http://codereview.appspot.com/3660043/diff/92001/java/gadgets/src/main/java/o... java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/ConcatProxyServlet.java:285: private static String formatHttpError(int status, String errorMessage, Uri uri) { On 2011/03/22 15:16:03, nikhilmadan23 wrote: > nit: add the space back Done. http://codereview.appspot.com/3660043/diff/92001/java/gadgets/src/test/java/o... File java/gadgets/src/test/java/org/apache/shindig/gadgets/servlet/ConcatProxyServletTest.java (right): http://codereview.appspot.com/3660043/diff/92001/java/gadgets/src/test/java/o... java/gadgets/src/test/java/org/apache/shindig/gadgets/servlet/ConcatProxyServletTest.java:27: import org.apache.shindig.common.servlet.HttpServletResponseRecorder; On 2011/03/22 15:16:03, nikhilmadan23 wrote: > nit: order Done http://codereview.appspot.com/3660043/diff/92001/java/gadgets/src/test/java/o... java/gadgets/src/test/java/org/apache/shindig/gadgets/servlet/ConcatProxyServletTest.java:300: On 2011/03/22 15:16:03, nikhilmadan23 wrote: > don't both the tests need a replay()? There is replay inside this function. http://codereview.appspot.com/3660043/diff/92001/java/gadgets/src/test/java/o... java/gadgets/src/test/java/org/apache/shindig/gadgets/servlet/ConcatProxyServletTest.java:304: String cacheControl = recorder.getHeader("Cache-Control"); On 2011/03/22 15:16:03, nikhilmadan23 wrote: > use HttpResponse.getCacheControlMaxAge instead? Made changes as per offline discussion http://codereview.appspot.com/3660043/diff/92001/java/gadgets/src/test/java/o... java/gadgets/src/test/java/org/apache/shindig/gadgets/servlet/ConcatProxyServletTest.java:324: assertEquals((int) (HttpResponse.defaultTtl / 1000), Integer.decode(cacheValue), 10); On 2011/03/22 14:24:28, satya3656 wrote: > can you add a comment on why /1000 is needed here. Done.
Sign in to reply to this message.
http://codereview.appspot.com/3660043/diff/96001/java/gadgets/src/main/java/o... File java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/ConcatProxyServlet.java (right): http://codereview.appspot.com/3660043/diff/96001/java/gadgets/src/main/java/o... java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/ConcatProxyServlet.java:126: Long minCacheTtl = getMinCacheTtlAndFetchResources(response, concatUri, uriResponseMap); why not move this into the try catch as well? http://codereview.appspot.com/3660043/diff/96001/java/gadgets/src/main/java/o... java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/ConcatProxyServlet.java:165: * fetched resources & fill up uriResponse map with HttpResponses & gadget exceptions. fetched -> fetches and fill -> filles
Sign in to reply to this message.
http://codereview.appspot.com/3660043/diff/96001/java/gadgets/src/main/java/o... File java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/ConcatProxyServlet.java (right): http://codereview.appspot.com/3660043/diff/96001/java/gadgets/src/main/java/o... java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/ConcatProxyServlet.java:126: Long minCacheTtl = getMinCacheTtlAndFetchResources(response, concatUri, uriResponseMap); On 2011/03/23 15:26:23, nikhilmadan23 wrote: > why not move this into the try catch as well? As discussed offline..Not changing http://codereview.appspot.com/3660043/diff/96001/java/gadgets/src/main/java/o... java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/ConcatProxyServlet.java:165: * fetched resources & fill up uriResponse map with HttpResponses & gadget exceptions. On 2011/03/23 15:26:23, nikhilmadan23 wrote: > fetched -> fetches and fill -> filles Done.
Sign in to reply to this message.
lgtm
Sign in to reply to this message.
> > Description: > > Cache Ttl value is set to 1 hr for concat resource when it is > unversioned. This value should set to minimum of all > individual resources' cache ttl value present in concat resource > > > Please review this at http://codereview.appspot.com/3660043/ >
Sign in to reply to this message.
http://codereview.appspot.com/3660043/diff/88007/java/gadgets/src/main/java/o... File java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/ConcatProxyServlet.java (right): http://codereview.appspot.com/3660043/diff/88007/java/gadgets/src/main/java/o... java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/ConcatProxyServlet.java:125: HashMap<Uri, Object> uriResponseMap = new HashMap<Uri, Object>(); nits: for lvalue use Map<...>, and rvalue use Maps.newHashMap(); http://codereview.appspot.com/3660043/diff/88007/java/gadgets/src/main/java/o... java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/ConcatProxyServlet.java:165: * fetches resources & filles up uriResponse map with HttpResponses & gadget exceptions. s/filles/fills/ http://codereview.appspot.com/3660043/diff/88007/java/gadgets/src/main/java/o... java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/ConcatProxyServlet.java:232: } rather than doing all this preprocessing, wouldn't it be easier to just modify ConcatOutputStream to buffer all output, and flush() it at the end (in the close() method)? Doing so would be pretty convenient since cos.output(..., HttpResponse) is already provided the HttpResponse, whose min TTL could be used when sending headers.
Sign in to reply to this message.
Can you use chunked encoding to send cache control header after the body? http://en.wikipedia.org/wiki/Chunked_transfer_encoding On Fri, Mar 25, 2011 at 2:42 AM, <johnfargo@gmail.com> wrote: > > > http://codereview.appspot.com/3660043/diff/88007/java/gadgets/src/main/java/o... > > File > > java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/ConcatProxyServlet.java > (right): > > > http://codereview.appspot.com/3660043/diff/88007/java/gadgets/src/main/java/o... > > java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/ConcatProxyServlet.java:125: > HashMap<Uri, Object> uriResponseMap = new HashMap<Uri, Object>(); > nits: for lvalue use Map<...>, and rvalue use Maps.newHashMap(); > > > http://codereview.appspot.com/3660043/diff/88007/java/gadgets/src/main/java/o... > > > java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/ConcatProxyServlet.java:165: > * fetches resources & filles up uriResponse map with HttpResponses & > gadget exceptions. > s/filles/fills/ > > > http://codereview.appspot.com/3660043/diff/88007/java/gadgets/src/main/java/o... > > java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/ConcatProxyServlet.java:232: > } > rather than doing all this preprocessing, wouldn't it be easier to just > modify ConcatOutputStream to buffer all output, and flush() it at the > end (in the close() method)? > > Doing so would be pretty convenient since cos.output(..., HttpResponse) > is already provided the HttpResponse, whose min TTL could be used when > sending headers. > > > http://codereview.appspot.com/3660043/ >
Sign in to reply to this message.
http://codereview.appspot.com/3660043/diff/88007/java/gadgets/src/main/java/o... File java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/ConcatProxyServlet.java (right): http://codereview.appspot.com/3660043/diff/88007/java/gadgets/src/main/java/o... java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/ConcatProxyServlet.java:125: HashMap<Uri, Object> uriResponseMap = new HashMap<Uri, Object>(); On 2011/03/24 21:12:40, johnfargo wrote: > nits: for lvalue use Map<...>, and rvalue use Maps.newHashMap(); Removed http://codereview.appspot.com/3660043/diff/88007/java/gadgets/src/main/java/o... java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/ConcatProxyServlet.java:165: * fetches resources & filles up uriResponse map with HttpResponses & gadget exceptions. On 2011/03/24 21:12:40, johnfargo wrote: > s/filles/fills/ No more needed http://codereview.appspot.com/3660043/diff/88007/java/gadgets/src/main/java/o... java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/ConcatProxyServlet.java:232: } On 2011/03/24 21:12:40, johnfargo wrote: > rather than doing all this preprocessing, wouldn't it be easier to just modify > ConcatOutputStream to buffer all output, and flush() it at the end (in the > close() method)? > > Doing so would be pretty convenient since cos.output(..., HttpResponse) is > already provided the HttpResponse, whose min TTL could be used when sending > headers. Yes this makes change looks very small :)
Sign in to reply to this message.
LGTM one nit. thx! http://codereview.appspot.com/3660043/diff/105001/java/gadgets/src/main/java/... File java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/ConcatProxyServlet.java (right): http://codereview.appspot.com/3660043/diff/105001/java/gadgets/src/main/java/... java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/ConcatProxyServlet.java:237: concatUri.translateStatusRefresh(LONG_LIVED_REFRESH, minCacheTtl.intValue()), false); nit: >100 char
Sign in to reply to this message.
I also moved min CacheTtl value calculation outside of finally block so that cos.close() always executes. http://codereview.appspot.com/3660043/diff/105001/java/gadgets/src/main/java/... File java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/ConcatProxyServlet.java (right): http://codereview.appspot.com/3660043/diff/105001/java/gadgets/src/main/java/... java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/ConcatProxyServlet.java:237: concatUri.translateStatusRefresh(LONG_LIVED_REFRESH, minCacheTtl.intValue()), false); On 2011/03/25 20:04:29, johnfargo wrote: > nit: >100 char Done.
Sign in to reply to this message.
looks good. http://codereview.appspot.com/3660043/diff/88013/java/gadgets/src/main/java/o... File java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/ConcatProxyServlet.java (right): http://codereview.appspot.com/3660043/diff/88013/java/gadgets/src/main/java/o... java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/ConcatProxyServlet.java:210: minCacheTtl = Math.min(minCacheTtl, httpResp.getCacheTtl()); shouldnt u be checking ttl after rewriting?
Sign in to reply to this message.
http://codereview.appspot.com/3660043/diff/88013/java/gadgets/src/main/java/o... File java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/ConcatProxyServlet.java (right): http://codereview.appspot.com/3660043/diff/88013/java/gadgets/src/main/java/o... java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/ConcatProxyServlet.java:210: minCacheTtl = Math.min(minCacheTtl, httpResp.getCacheTtl()); On 2011/03/29 16:37:37, gagan.goku wrote: > shouldnt u be checking ttl after rewriting? Done.
Sign in to reply to this message.
lgtm
Sign in to reply to this message.
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
