On Mon, Jun 21, 2010 at 7:11 PM, <[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: > 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 >
