Adding capability in accel servlet to act as accelerating proxy

- Adding accel servlet as catch all servlet
- Modifying AccelUriManager to deal with all urls.

[gagan.goku, 2010-06-21 05:07:37]

Please review.

[zhoresh, 2010-06-21 21:50:25]

http://codereview.appspot.com/1712043/diff/5001/6005
File
java/gadgets/src/main/java/org/apache/shindig/gadgets/uri/AccelUriManager.java
(right):

http://codereview.appspot.com/1712043/diff/5001/6005#newcode65
java/gadgets/src/main/java/org/apache/shindig/gadgets/uri/AccelUriManager.java:65:
String accelPath = config.getString(container, PROXY_PATH_PARAM);
Shouldn't it be accel_path?

http://codereview.appspot.com/1712043/diff/5001/6005#newcode67
java/gadgets/src/main/java/org/apache/shindig/gadgets/uri/AccelUriManager.java:67:
return accelHost.equals(requestUri.getAuthority()) &&
I don't think we can promise that the authority will always match

[johnfargo, 2010-06-21 22:55:13]

http://codereview.appspot.com/1712043/diff/5001/6003
File
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/AccelHandler.java
(right):

http://codereview.appspot.com/1712043/diff/5001/6003#newcode123
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/AccelHandler.java:123:
Uri normalizedUri = Uri.parse(proxiedUri.getQueryParameter(
surround this with try/catch(UriException) for error handling.

http://codereview.appspot.com/1712043/diff/5001/6005
File
java/gadgets/src/main/java/org/apache/shindig/gadgets/uri/AccelUriManager.java
(right):

http://codereview.appspot.com/1712043/diff/5001/6005#newcode50
java/gadgets/src/main/java/org/apache/shindig/gadgets/uri/AccelUriManager.java:50:
public Uri parseAndNormalize(Gadget gadget, Uri requestUri) throws
GadgetException {
While we're at it, we should refactor this class a bit to better reflect the
needs of Accel:
1. parse method should take an HttpRequest rather than a Gadget.

then perhaps (these can be in a followup as well)
2. Don't extend DefaultProxyUriManager, just @Inject a ProxyUriManager to which
you delegate when useful.
3. [optional, but fits the pattern] Make AccelUriManager an interface, with
DefaultAccelUriManager implementing it.
^^ these two help isolate AccelUriManager from potential future issues having to
do w/ DefaultProxyUriManager impl changes.

http://codereview.appspot.com/1712043/diff/5001/6005#newcode65
java/gadgets/src/main/java/org/apache/shindig/gadgets/uri/AccelUriManager.java:65:
String accelPath = config.getString(container, PROXY_PATH_PARAM);
You should also just calculate these once in the ctor.

http://codereview.appspot.com/1712043/diff/5001/6005#newcode67
java/gadgets/src/main/java/org/apache/shindig/gadgets/uri/AccelUriManager.java:67:
return accelHost.equals(requestUri.getAuthority()) &&
+1, at least for testing purposes. You might also, out of conservatism, choose
to equalsIgnoreCase

http://codereview.appspot.com/1712043/diff/5001/6004
File
java/gadgets/src/main/java/org/apache/shindig/gadgets/uri/DefaultProxyUriManager.java
(right):

http://codereview.appspot.com/1712043/diff/5001/6004#newcode66
java/gadgets/src/main/java/org/apache/shindig/gadgets/uri/DefaultProxyUriManager.java:66:
protected final ContainerConfig config;
not needed; just store your own ContainerConfig reference in AccelUriManager.

[gagan.goku, 2010-06-22 02:11:18]

http://codereview.appspot.com/1712043/diff/5001/6003
File
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/AccelHandler.java
(right):

http://codereview.appspot.com/1712043/diff/5001/6003#newcode123
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/AccelHandler.java:123:
Uri normalizedUri = Uri.parse(proxiedUri.getQueryParameter(
On 2010/06/21 22:55:13, johnfargo wrote:
> surround this with try/catch(UriException) for error handling.

Our error handling strategy should be 2 fold in this case:
1) In case there is a problem parsing the uri, or fetching content for a given
uri, there is not much we can do about it. I guess throwing 404+ is our only
option there.
Or we can try and redirect the guy to original page, but lets not think of that
case now.
2) In case we were able to fetch the content for the given url, we should
definitely return this content in case of any errors.
So we make sure our rewriting pipeline doesn't mess up. If it does, we fallback
to this content.

currently this method already throws GadgetException. Plus, the errors
encountered in this function seem to be non recoverable for now. how do you want
it refactored ?

http://codereview.appspot.com/1712043/diff/5001/6005
File
java/gadgets/src/main/java/org/apache/shindig/gadgets/uri/AccelUriManager.java
(right):

http://codereview.appspot.com/1712043/diff/5001/6005#newcode50
java/gadgets/src/main/java/org/apache/shindig/gadgets/uri/AccelUriManager.java:50:
public Uri parseAndNormalize(Gadget gadget, Uri requestUri) throws
GadgetException {
On 2010/06/21 22:55:13, johnfargo wrote:
> While we're at it, we should refactor this class a bit to better reflect the
> needs of Accel:
> 1. parse method should take an HttpRequest rather than a Gadget.
but i need the gadget param as non null to be able to generate valid ProxyUri's
:(
This is the only way i could figure out to send a non null value for gadget when
generating a proxy uri for say:
www.example.org/a.html

> 
> then perhaps (these can be in a followup as well)
> 2. Don't extend DefaultProxyUriManager, just @Inject a ProxyUriManager to
which
> you delegate when useful.
> 3. [optional, but fits the pattern] Make AccelUriManager an interface, with
> DefaultAccelUriManager implementing it.
> ^^ these two help isolate AccelUriManager from potential future issues having
to
> do w/ DefaultProxyUriManager impl changes.
Done.

http://codereview.appspot.com/1712043/diff/5001/6005#newcode65
java/gadgets/src/main/java/org/apache/shindig/gadgets/uri/AccelUriManager.java:65:
String accelPath = config.getString(container, PROXY_PATH_PARAM);
On 2010/06/21 21:50:25, zhoresh wrote:
> Shouldn't it be accel_path?

you mean ACCEL_PATH ?

http://codereview.appspot.com/1712043/diff/5001/6005#newcode65
java/gadgets/src/main/java/org/apache/shindig/gadgets/uri/AccelUriManager.java:65:
String accelPath = config.getString(container, PROXY_PATH_PARAM);
On 2010/06/21 22:55:13, johnfargo wrote:
> You should also just calculate these once in the ctor.

Done.

http://codereview.appspot.com/1712043/diff/5001/6005#newcode67
java/gadgets/src/main/java/org/apache/shindig/gadgets/uri/AccelUriManager.java:67:
return accelHost.equals(requestUri.getAuthority()) &&
On 2010/06/21 21:50:25, zhoresh wrote:
> I don't think we can promise that the authority will always match

for now this is a hack because i have only 1 accel host.
later when i have multiple, il check for something like this:
accelHosts.contains(requestUri.getAuthority())

http://codereview.appspot.com/1712043/diff/5001/6004
File
java/gadgets/src/main/java/org/apache/shindig/gadgets/uri/DefaultProxyUriManager.java
(right):

http://codereview.appspot.com/1712043/diff/5001/6004#newcode66
java/gadgets/src/main/java/org/apache/shindig/gadgets/uri/DefaultProxyUriManager.java:66:
protected final ContainerConfig config;
On 2010/06/21 22:55:13, johnfargo wrote:
> not needed; just store your own ContainerConfig reference in AccelUriManager.

Done.

[fargo, 2010-06-22 02:31:12]

On Mon, Jun 21, 2010 at 7:11 PM, <gagan.goku@gmail.com> wrote:

>
> http://codereview.appspot.com/1712043/diff/5001/6003
> File
>
>
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/AccelHandler.java
> (right):
>
> http://codereview.appspot.com/1712043/diff/5001/6003#newcode123
>
>
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/AccelHandler.java:123:
> Uri normalizedUri = Uri.parse(proxiedUri.getQueryParameter(
> On 2010/06/21 22:55:13, johnfargo wrote:
>
>> surround this with try/catch(UriException) for error handling.
>>
>
> Our error handling strategy should be 2 fold in this case:
> 1) In case there is a problem parsing the uri, or fetching content for a
> given uri, there is not much we can do about it. I guess throwing 404+
> is our only option there.
>

Agreed. My suggestion to catch UriException comes from the fact that
UriException is a RuntimeException, so if left uncaught will end up as a
500. This is for historical reasons, sadly.


> Or we can try and redirect the guy to original page, but lets not think
> of that case now.
>

SGTM.


> 2) In case we were able to fetch the content for the given url, we
> should definitely return this content in case of any errors.
> So we make sure our rewriting pipeline doesn't mess up. If it does, we
> fallback to this content.
>
> currently this method already throws GadgetException. Plus, the errors
> encountered in this function seem to be non recoverable for now. how do
> you want it refactored ?


@see comments below.


>
>
> http://codereview.appspot.com/1712043/diff/5001/6005
> File
>
> java/gadgets/src/main/java/org/apache/shindig/gadgets/uri/AccelUriManager.java
> (right):
>
> http://codereview.appspot.com/1712043/diff/5001/6005#newcode50
>
>
java/gadgets/src/main/java/org/apache/shindig/gadgets/uri/AccelUriManager.java:50:
> public Uri parseAndNormalize(Gadget gadget, Uri requestUri) throws
> GadgetException {
> On 2010/06/21 22:55:13, johnfargo wrote:
>
>> While we're at it, we should refactor this class a bit to better
>>
> reflect the
>
>> needs of Accel:
>> 1. parse method should take an HttpRequest rather than a Gadget.
>>
> but i need the gadget param as non null to be able to generate valid
> ProxyUri's :(
>

Right, I'm only suggesting to set the contract for AccelUriManager to accept
HttpRequest. The impl will still convert to a Gadget for processing by the
delegated [Default]ProxyUriManager. This will allow us to slowly peel the
Gadget-ness away from other implementations w/o infecting new interfaces.


> This is the only way i could figure out to send a non null value for
> gadget when generating a proxy uri for say:
> www.example.org/a.html
>
>
>
>  then perhaps (these can be in a followup as well)
>> 2. Don't extend DefaultProxyUriManager, just @Inject a ProxyUriManager
>>
> to which
>
>> you delegate when useful.
>> 3. [optional, but fits the pattern] Make AccelUriManager an interface,
>>
> with
>
>> DefaultAccelUriManager implementing it.
>> ^^ these two help isolate AccelUriManager from potential future issues
>>
> having to
>
>> do w/ DefaultProxyUriManager impl changes.
>>
> Done.
>
>
> http://codereview.appspot.com/1712043/diff/5001/6005#newcode65
>
>
java/gadgets/src/main/java/org/apache/shindig/gadgets/uri/AccelUriManager.java:65:
> String accelPath = config.getString(container, PROXY_PATH_PARAM);
> On 2010/06/21 21:50:25, zhoresh wrote:
>
>> Shouldn't it be accel_path?
>>
>
> you mean ACCEL_PATH ?
>
>
> http://codereview.appspot.com/1712043/diff/5001/6005#newcode65
>
>
java/gadgets/src/main/java/org/apache/shindig/gadgets/uri/AccelUriManager.java:65:
> String accelPath = config.getString(container, PROXY_PATH_PARAM);
> On 2010/06/21 22:55:13, johnfargo wrote:
>
>> You should also just calculate these once in the ctor.
>>
>
> Done.
>
>
> http://codereview.appspot.com/1712043/diff/5001/6005#newcode67
>
>
java/gadgets/src/main/java/org/apache/shindig/gadgets/uri/AccelUriManager.java:67:
> return accelHost.equals(requestUri.getAuthority()) &&
> On 2010/06/21 21:50:25, zhoresh wrote:
>
>> I don't think we can promise that the authority will always match
>>
>
> for now this is a hack because i have only 1 accel host.
> later when i have multiple, il check for something like this:
> accelHosts.contains(requestUri.getAuthority())
>
>
> http://codereview.appspot.com/1712043/diff/5001/6004
> File
>
>
java/gadgets/src/main/java/org/apache/shindig/gadgets/uri/DefaultProxyUriManager.java
> (right):
>
> http://codereview.appspot.com/1712043/diff/5001/6004#newcode66
>
>
java/gadgets/src/main/java/org/apache/shindig/gadgets/uri/DefaultProxyUriManager.java:66:
> protected final ContainerConfig config;
> On 2010/06/21 22:55:13, johnfargo wrote:
>
>> not needed; just store your own ContainerConfig reference in
>>
> AccelUriManager.
>
> Done.
>
>
> http://codereview.appspot.com/1712043/show
>

[gagan.goku, 2010-06-22 03:08:17]

Done.

On 2010/06/22 02:31:12, fargo wrote:
> On Mon, Jun 21, 2010 at 7:11 PM, <mailto:gagan.goku@gmail.com> wrote:
> 
> >
> > http://codereview.appspot.com/1712043/diff/5001/6003
> > File
> >
> >
>
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/AccelHandler.java
> > (right):
> >
> > http://codereview.appspot.com/1712043/diff/5001/6003#newcode123
> >
> >
>
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/AccelHandler.java:123:
> > Uri normalizedUri = Uri.parse(proxiedUri.getQueryParameter(
> > On 2010/06/21 22:55:13, johnfargo wrote:
> >
> >> surround this with try/catch(UriException) for error handling.
> >>
> >
> > Our error handling strategy should be 2 fold in this case:
> > 1) In case there is a problem parsing the uri, or fetching content for a
> > given uri, there is not much we can do about it. I guess throwing 404+
> > is our only option there.
> >
> 
> Agreed. My suggestion to catch UriException comes from the fact that
> UriException is a RuntimeException, so if left uncaught will end up as a
> 500. This is for historical reasons, sadly.
> 
> 
> > Or we can try and redirect the guy to original page, but lets not think
> > of that case now.
> >
> 
> SGTM.
> 
> 
> > 2) In case we were able to fetch the content for the given url, we
> > should definitely return this content in case of any errors.
> > So we make sure our rewriting pipeline doesn't mess up. If it does, we
> > fallback to this content.
> >
> > currently this method already throws GadgetException. Plus, the errors
> > encountered in this function seem to be non recoverable for now. how do
> > you want it refactored ?
> 
> 
> @see comments below.
> 
> 
> >
> >
> > http://codereview.appspot.com/1712043/diff/5001/6005
> > File
> >
> >
java/gadgets/src/main/java/org/apache/shindig/gadgets/uri/AccelUriManager.java
> > (right):
> >
> > http://codereview.appspot.com/1712043/diff/5001/6005#newcode50
> >
> >
>
java/gadgets/src/main/java/org/apache/shindig/gadgets/uri/AccelUriManager.java:50:
> > public Uri parseAndNormalize(Gadget gadget, Uri requestUri) throws
> > GadgetException {
> > On 2010/06/21 22:55:13, johnfargo wrote:
> >
> >> While we're at it, we should refactor this class a bit to better
> >>
> > reflect the
> >
> >> needs of Accel:
> >> 1. parse method should take an HttpRequest rather than a Gadget.
> >>
> > but i need the gadget param as non null to be able to generate valid
> > ProxyUri's :(
> >
> 
> Right, I'm only suggesting to set the contract for AccelUriManager to accept
> HttpRequest. The impl will still convert to a Gadget for processing by the
> delegated [Default]ProxyUriManager. This will allow us to slowly peel the
> Gadget-ness away from other implementations w/o infecting new interfaces.
> 
> 
> > This is the only way i could figure out to send a non null value for
> > gadget when generating a proxy uri for say:
> > http://www.example.org/a.html
> >
> >
> >
> >  then perhaps (these can be in a followup as well)
> >> 2. Don't extend DefaultProxyUriManager, just @Inject a ProxyUriManager
> >>
> > to which
> >
> >> you delegate when useful.
> >> 3. [optional, but fits the pattern] Make AccelUriManager an interface,
> >>
> > with
> >
> >> DefaultAccelUriManager implementing it.
> >> ^^ these two help isolate AccelUriManager from potential future issues
> >>
> > having to
> >
> >> do w/ DefaultProxyUriManager impl changes.
> >>
> > Done.
> >
> >
> > http://codereview.appspot.com/1712043/diff/5001/6005#newcode65
> >
> >
>
java/gadgets/src/main/java/org/apache/shindig/gadgets/uri/AccelUriManager.java:65:
> > String accelPath = config.getString(container, PROXY_PATH_PARAM);
> > On 2010/06/21 21:50:25, zhoresh wrote:
> >
> >> Shouldn't it be accel_path?
> >>
> >
> > you mean ACCEL_PATH ?
> >
> >
> > http://codereview.appspot.com/1712043/diff/5001/6005#newcode65
> >
> >
>
java/gadgets/src/main/java/org/apache/shindig/gadgets/uri/AccelUriManager.java:65:
> > String accelPath = config.getString(container, PROXY_PATH_PARAM);
> > On 2010/06/21 22:55:13, johnfargo wrote:
> >
> >> You should also just calculate these once in the ctor.
> >>
> >
> > Done.
> >
> >
> > http://codereview.appspot.com/1712043/diff/5001/6005#newcode67
> >
> >
>
java/gadgets/src/main/java/org/apache/shindig/gadgets/uri/AccelUriManager.java:67:
> > return accelHost.equals(requestUri.getAuthority()) &&
> > On 2010/06/21 21:50:25, zhoresh wrote:
> >
> >> I don't think we can promise that the authority will always match
> >>
> >
> > for now this is a hack because i have only 1 accel host.
> > later when i have multiple, il check for something like this:
> > accelHosts.contains(requestUri.getAuthority())
> >
> >
> > http://codereview.appspot.com/1712043/diff/5001/6004
> > File
> >
> >
>
java/gadgets/src/main/java/org/apache/shindig/gadgets/uri/DefaultProxyUriManager.java
> > (right):
> >
> > http://codereview.appspot.com/1712043/diff/5001/6004#newcode66
> >
> >
>
java/gadgets/src/main/java/org/apache/shindig/gadgets/uri/DefaultProxyUriManager.java:66:
> > protected final ContainerConfig config;
> > On 2010/06/21 22:55:13, johnfargo wrote:
> >
> >> not needed; just store your own ContainerConfig reference in
> >>
> > AccelUriManager.
> >
> > Done.
> >
> >
> > http://codereview.appspot.com/1712043/show
> >
>

[johnfargo, 2010-06-22 03:36:33]

final nits, should just be 1 more iteration.

http://codereview.appspot.com/1712043/diff/19001/20005
File
java/gadgets/src/main/java/org/apache/shindig/gadgets/uri/AccelUriManager.java
(right):

http://codereview.appspot.com/1712043/diff/19001/20005#newcode32
java/gadgets/src/main/java/org/apache/shindig/gadgets/uri/AccelUriManager.java:32:
final String PROXY_PATH_PARAM = DefaultProxyUriManager.PROXY_PATH_PARAM;
can be public, should be static.

http://codereview.appspot.com/1712043/diff/19001/20006
File
java/gadgets/src/main/java/org/apache/shindig/gadgets/uri/DefaultAccelUriManager.java
(right):

http://codereview.appspot.com/1712043/diff/19001/20006#newcode46
java/gadgets/src/main/java/org/apache/shindig/gadgets/uri/DefaultAccelUriManager.java:46:
ACCEL_PATH = config.getString(CONTAINER,
DefaultProxyUriManager.PROXY_PATH_PARAM);
nit: for local non-static variables, use camelCase variables starting w/
lowerCase letters

http://codereview.appspot.com/1712043/diff/19001/20003
File
java/gadgets/src/test/java/org/apache/shindig/gadgets/uri/AccelUriManagerTest.java
(right):

http://codereview.appspot.com/1712043/diff/19001/20003#newcode27
java/gadgets/src/test/java/org/apache/shindig/gadgets/uri/AccelUriManagerTest.java:27:
* TODO: Complete or remove.
I'd remove this now; AccelUriManager is not testable. DefaultAccelUriManager
however is ;)

[gagan.goku, 2010-06-22 04:59:45]

http://codereview.appspot.com/1712043/diff/19001/20005
File
java/gadgets/src/main/java/org/apache/shindig/gadgets/uri/AccelUriManager.java
(right):

http://codereview.appspot.com/1712043/diff/19001/20005#newcode32
java/gadgets/src/main/java/org/apache/shindig/gadgets/uri/AccelUriManager.java:32:
final String PROXY_PATH_PARAM = DefaultProxyUriManager.PROXY_PATH_PARAM;
On 2010/06/22 03:36:33, johnfargo wrote:
> can be public, should be static.

Done.

http://codereview.appspot.com/1712043/diff/19001/20006
File
java/gadgets/src/main/java/org/apache/shindig/gadgets/uri/DefaultAccelUriManager.java
(right):

http://codereview.appspot.com/1712043/diff/19001/20006#newcode46
java/gadgets/src/main/java/org/apache/shindig/gadgets/uri/DefaultAccelUriManager.java:46:
ACCEL_PATH = config.getString(CONTAINER,
DefaultProxyUriManager.PROXY_PATH_PARAM);
On 2010/06/22 03:36:33, johnfargo wrote:
> nit: for local non-static variables, use camelCase variables starting w/
> lowerCase letters

Done.

http://codereview.appspot.com/1712043/diff/19001/20003
File
java/gadgets/src/test/java/org/apache/shindig/gadgets/uri/AccelUriManagerTest.java
(right):

http://codereview.appspot.com/1712043/diff/19001/20003#newcode27
java/gadgets/src/test/java/org/apache/shindig/gadgets/uri/AccelUriManagerTest.java:27:
* TODO: Complete or remove.
On 2010/06/22 03:36:33, johnfargo wrote:
> I'd remove this now; AccelUriManager is not testable. DefaultAccelUriManager
> however is ;)

added test for DefaultAccelUriManager. file reverted

[johnfargo, 2010-06-22 23:23:40]

LGTM, committed.

On 2010/06/22 04:59:45, gagan.goku wrote:
> http://codereview.appspot.com/1712043/diff/19001/20005
> File
> java/gadgets/src/main/java/org/apache/shindig/gadgets/uri/AccelUriManager.java
> (right):
> 
> http://codereview.appspot.com/1712043/diff/19001/20005#newcode32
>
java/gadgets/src/main/java/org/apache/shindig/gadgets/uri/AccelUriManager.java:32:
> final String PROXY_PATH_PARAM = DefaultProxyUriManager.PROXY_PATH_PARAM;
> On 2010/06/22 03:36:33, johnfargo wrote:
> > can be public, should be static.
> 
> Done.
> 
> http://codereview.appspot.com/1712043/diff/19001/20006
> File
>
java/gadgets/src/main/java/org/apache/shindig/gadgets/uri/DefaultAccelUriManager.java
> (right):
> 
> http://codereview.appspot.com/1712043/diff/19001/20006#newcode46
>
java/gadgets/src/main/java/org/apache/shindig/gadgets/uri/DefaultAccelUriManager.java:46:
> ACCEL_PATH = config.getString(CONTAINER,
> DefaultProxyUriManager.PROXY_PATH_PARAM);
> On 2010/06/22 03:36:33, johnfargo wrote:
> > nit: for local non-static variables, use camelCase variables starting w/
> > lowerCase letters
> 
> Done.
> 
> http://codereview.appspot.com/1712043/diff/19001/20003
> File
>
java/gadgets/src/test/java/org/apache/shindig/gadgets/uri/AccelUriManagerTest.java
> (right):
> 
> http://codereview.appspot.com/1712043/diff/19001/20003#newcode27
>
java/gadgets/src/test/java/org/apache/shindig/gadgets/uri/AccelUriManagerTest.java:27:
> * TODO: Complete or remove.
> On 2010/06/22 03:36:33, johnfargo wrote:
> > I'd remove this now; AccelUriManager is not testable. DefaultAccelUriManager
> > however is ;)
> 
> added test for DefaultAccelUriManager. file reverted

[johnfargo, 2010-06-22 23:23:55]

(I made this change myself before committing)

http://codereview.appspot.com/1712043/diff/7002/32004
File
java/gadgets/src/test/java/org/apache/shindig/gadgets/uri/DefaultAccelUriManagerTest.java
(right):

http://codereview.appspot.com/1712043/diff/7002/32004#newcode66
java/gadgets/src/test/java/org/apache/shindig/gadgets/uri/DefaultAccelUriManagerTest.java:66:
uriManager.parseAndNormalize(uri).toString());
to make this (and the next 2) tests less prone to flakiness, let's do an
assertEquals on the Uris themselves (ie. parse the first String)

[gagan.goku, 2010-06-22 23:25:46]

On 2010/06/22 23:23:55, johnfargo wrote:
> (I made this change myself before committing)
> 
> http://codereview.appspot.com/1712043/diff/7002/32004
> File
>
java/gadgets/src/test/java/org/apache/shindig/gadgets/uri/DefaultAccelUriManagerTest.java
> (right):
> 
> http://codereview.appspot.com/1712043/diff/7002/32004#newcode66
>
java/gadgets/src/test/java/org/apache/shindig/gadgets/uri/DefaultAccelUriManagerTest.java:66:
> uriManager.parseAndNormalize(uri).toString());
> to make this (and the next 2) tests less prone to flakiness, let's do an
> assertEquals on the Uris themselves (ie. parse the first String)

Thanks John