Done.
On 2010/06/22 02:31:12, fargo wrote:
On Mon, Jun 21, 2010 at 7:11 PM, <mailto:[email protected]> 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
>
http://codereview.appspot.com/1712043/show
