Redesign '/concat' servlet' - Fetch resources concurrently

Redesign '/concat' servlet' to add the flexibility of fetching the concat resources concurrently.

Design Change:
---------------
The proposed 'Shindig' change employs Java (1.5) Concurrent constructs like 'Executor', 'FutureTask', 'Callable' to create a FutureTask of Callable (HttpRequest) object and submits this future Task to the appropriate Executor. The default Executor implementation is a sequential one and can be 'binded' to appropriate 'Threded' implementation in the GuiceModule.

[johnfargo, 2010-06-21 17:07:40]

Hi Vikas:

This change looks excellent. My comments are all formatting nits, which I can
take care of before committing. Thanks!

--j

http://codereview.appspot.com/1681044/diff/1/2
File src/main/java/org/apache/shindig/gadgets/servlet/ConcatProxyServlet.java
(right):

http://codereview.appspot.com/1681044/diff/1/2#newcode83
src/main/java/org/apache/shindig/gadgets/servlet/ConcatProxyServlet.java:83: 
nit: excess newline

http://codereview.appspot.com/1681044/diff/1/2#newcode172
src/main/java/org/apache/shindig/gadgets/servlet/ConcatProxyServlet.java:172:
new ArrayList<Pair<Uri, FutureTask<RequestContext>>>();
nit: 4-space indent

http://codereview.appspot.com/1681044/diff/1/2#newcode178
src/main/java/org/apache/shindig/gadgets/servlet/ConcatProxyServlet.java:178:
new FutureTask<RequestContext>(new HttpFetchCallable(httpReq));
same

http://codereview.appspot.com/1681044/diff/1/3
File
src/test/java/org/apache/shindig/gadgets/servlet/ConcatProxyServletTest.java
(right):

http://codereview.appspot.com/1681044/diff/1/3#newcode61
src/test/java/org/apache/shindig/gadgets/servlet/ConcatProxyServletTest.java:61:
private final Executor SequentialExecutor = new Executor() {
nit: start w/ lowercase

http://codereview.appspot.com/1681044/diff/1/3#newcode68
src/test/java/org/apache/shindig/gadgets/servlet/ConcatProxyServletTest.java:68:
private final Executor ThreadedExecutor = new Executor() {
same

http://codereview.appspot.com/1681044/diff/1/3#newcode154
src/test/java/org/apache/shindig/gadgets/servlet/ConcatProxyServletTest.java:154:
assertEquals(200, recorder.getHttpStatusCode());
this method can be consolidated w/ the one above, with just an executor passed
in

[johnfargo, 2010-06-21 17:53:05]

Committed.

On 2010/06/21 17:07:40, johnfargo wrote:
> Hi Vikas:
> 
> This change looks excellent. My comments are all formatting nits, which I can
> take care of before committing. Thanks!
> 
> --j
> 
> http://codereview.appspot.com/1681044/diff/1/2
> File src/main/java/org/apache/shindig/gadgets/servlet/ConcatProxyServlet.java
> (right):
> 
> http://codereview.appspot.com/1681044/diff/1/2#newcode83
> src/main/java/org/apache/shindig/gadgets/servlet/ConcatProxyServlet.java:83: 
> nit: excess newline
> 
> http://codereview.appspot.com/1681044/diff/1/2#newcode172
> src/main/java/org/apache/shindig/gadgets/servlet/ConcatProxyServlet.java:172:
> new ArrayList<Pair<Uri, FutureTask<RequestContext>>>();
> nit: 4-space indent
> 
> http://codereview.appspot.com/1681044/diff/1/2#newcode178
> src/main/java/org/apache/shindig/gadgets/servlet/ConcatProxyServlet.java:178:
> new FutureTask<RequestContext>(new HttpFetchCallable(httpReq));
> same
> 
> http://codereview.appspot.com/1681044/diff/1/3
> File
> src/test/java/org/apache/shindig/gadgets/servlet/ConcatProxyServletTest.java
> (right):
> 
> http://codereview.appspot.com/1681044/diff/1/3#newcode61
>
src/test/java/org/apache/shindig/gadgets/servlet/ConcatProxyServletTest.java:61:
> private final Executor SequentialExecutor = new Executor() {
> nit: start w/ lowercase
> 
> http://codereview.appspot.com/1681044/diff/1/3#newcode68
>
src/test/java/org/apache/shindig/gadgets/servlet/ConcatProxyServletTest.java:68:
> private final Executor ThreadedExecutor = new Executor() {
> same
> 
> http://codereview.appspot.com/1681044/diff/1/3#newcode154
>
src/test/java/org/apache/shindig/gadgets/servlet/ConcatProxyServletTest.java:154:
> assertEquals(200, recorder.getHttpStatusCode());
> this method can be consolidated w/ the one above, with just an executor passed
> in

[vikaas.arora, 2010-06-22 03:11:26]

On 2010/06/21 17:53:05, johnfargo wrote:
> Committed.
> 
Thanks John

> On 2010/06/21 17:07:40, johnfargo wrote:
> > Hi Vikas:
> > 
> > This change looks excellent. My comments are all formatting nits, which I
can
> > take care of before committing. Thanks!
> > 
> > --j
> > 
> > http://codereview.appspot.com/1681044/diff/1/2
> > File
src/main/java/org/apache/shindig/gadgets/servlet/ConcatProxyServlet.java
> > (right):
> > 
> > http://codereview.appspot.com/1681044/diff/1/2#newcode83
> > src/main/java/org/apache/shindig/gadgets/servlet/ConcatProxyServlet.java:83:

> > nit: excess newline
> > 
> > http://codereview.appspot.com/1681044/diff/1/2#newcode172
> >
src/main/java/org/apache/shindig/gadgets/servlet/ConcatProxyServlet.java:172:
> > new ArrayList<Pair<Uri, FutureTask<RequestContext>>>();
> > nit: 4-space indent
> > 

Seems I need to set some Eclipse settings (formatting) to add 4-space indent for
line breaks.


> > http://codereview.appspot.com/1681044/diff/1/2#newcode178
> >
src/main/java/org/apache/shindig/gadgets/servlet/ConcatProxyServlet.java:178:
> > new FutureTask<RequestContext>(new HttpFetchCallable(httpReq));
> > same
> > 
> > http://codereview.appspot.com/1681044/diff/1/3
> > File
> > src/test/java/org/apache/shindig/gadgets/servlet/ConcatProxyServletTest.java
> > (right):
> > 
> > http://codereview.appspot.com/1681044/diff/1/3#newcode61
> >
>
src/test/java/org/apache/shindig/gadgets/servlet/ConcatProxyServletTest.java:61:
> > private final Executor SequentialExecutor = new Executor() {
> > nit: start w/ lowercase
> > 

In the first version of code, I had the final vars starting with lower case
(just like non-final vars). Eclipse was giving me soft worm indicators and
suggested to use final var names starting with 'Caps' letter. Seems another
Eclipse settings needs to be tuned.


> > http://codereview.appspot.com/1681044/diff/1/3#newcode68
> >
>
src/test/java/org/apache/shindig/gadgets/servlet/ConcatProxyServletTest.java:68:
> > private final Executor ThreadedExecutor = new Executor() {
> > same
> > 
> > http://codereview.appspot.com/1681044/diff/1/3#newcode154
> >
>
src/test/java/org/apache/shindig/gadgets/servlet/ConcatProxyServletTest.java:154:
> > assertEquals(200, recorder.getHttpStatusCode());
> > this method can be consolidated w/ the one above, with just an executor
passed
> > in