Adding DomainBalancingUriRewriter

DomainBalancingUriRewriter: rewriter that does basic domain balancing over
links so that all resources are not fetched from same host (browsers usually open only 3 connections per host, so having multiple hosts can improve page load time)

JIRA issue: https://issues.apache.org/jira/browse/SHINDIG-1373

[zhoresh, 2010-06-14 21:03:00]

http://codereview.appspot.com/1674041/diff/5001/6007
File
java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/DomainBalancingUriVisitor.java
(right):

http://codereview.appspot.com/1674041/diff/5001/6007#newcode44
java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/DomainBalancingUriVisitor.java:44:
public class DomainBalancingUriVisitor extends AbsolutePathReferenceVisitor {
Can you move this to separate review?
At this point it doesn't really do much, so maybe explain what you are trying to
do here.

http://codereview.appspot.com/1674041/diff/5001/6007#newcode82
java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/DomainBalancingUriVisitor.java:82:
Map<Node, Uri> nodeToAbsoluteUri = new HashMap<Node, Uri>(nodes.size());
Maybe you can just use lists instead of maps. Unless I miss something, I think
getRewrittenUris just go over the urls in order.

http://codereview.appspot.com/1674041/diff/5001/6007#newcode142
java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/DomainBalancingUriVisitor.java:142:
* GGS or not. Uri's rewritten by other rewriters should not be rewritten as
please use Shindig instead of GGS

http://codereview.appspot.com/1674041/diff/5001/6007#newcode162
java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/DomainBalancingUriVisitor.java:162:
public void registerAccelHosts(List<String> accelHosts) {
I prefer injected constructor over injected set functions (and don't you also
need to name the string list?)

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

http://codereview.appspot.com/1674041/diff/5001/6006#newcode56
java/gadgets/src/main/java/org/apache/shindig/gadgets/uri/AccelUriManager.java:56:
requestUri = new UriBuilder()
Can this be merged with RoxyUriManager? Basically you want similar code so it
can hanle chain-param style the same way.

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

http://codereview.appspot.com/1674041/diff/5001/6005#newcode122
java/gadgets/src/main/java/org/apache/shindig/gadgets/uri/DefaultProxyUriManager.java:122:
uri.setScheme(config.getString(container, PROXY_SCHEME));
No need to set schema, it is taken from the parent page

[gagan.goku, 2010-06-14 23:22:11]

On 2010/06/14 21:03:00, zhoresh wrote:
> http://codereview.appspot.com/1674041/diff/5001/6007
> File
>
java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/DomainBalancingUriVisitor.java
> (right):
> 
> http://codereview.appspot.com/1674041/diff/5001/6007#newcode44
>
java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/DomainBalancingUriVisitor.java:44:
> public class DomainBalancingUriVisitor extends AbsolutePathReferenceVisitor {
> Can you move this to separate review?
> At this point it doesn't really do much, so maybe explain what you are trying
to
> do here.
> 
> http://codereview.appspot.com/1674041/diff/5001/6007#newcode82
>
java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/DomainBalancingUriVisitor.java:82:
> Map<Node, Uri> nodeToAbsoluteUri = new HashMap<Node, Uri>(nodes.size());
> Maybe you can just use lists instead of maps. Unless I miss something, I think
> getRewrittenUris just go over the urls in order.
> 
> http://codereview.appspot.com/1674041/diff/5001/6007#newcode142
>
java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/DomainBalancingUriVisitor.java:142:
> * GGS or not. Uri's rewritten by other rewriters should not be rewritten as
> please use Shindig instead of GGS
> 
> http://codereview.appspot.com/1674041/diff/5001/6007#newcode162
>
java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/DomainBalancingUriVisitor.java:162:
> public void registerAccelHosts(List<String> accelHosts) {
> I prefer injected constructor over injected set functions (and don't you also
> need to name the string list?)
> 
> http://codereview.appspot.com/1674041/diff/5001/6006
> File
> java/gadgets/src/main/java/org/apache/shindig/gadgets/uri/AccelUriManager.java
> (right):
> 
> http://codereview.appspot.com/1674041/diff/5001/6006#newcode56
>
java/gadgets/src/main/java/org/apache/shindig/gadgets/uri/AccelUriManager.java:56:
> requestUri = new UriBuilder()
> Can this be merged with RoxyUriManager? Basically you want similar code so it
> can hanle chain-param style the same way.
> 
> http://codereview.appspot.com/1674041/diff/5001/6005
> File
>
java/gadgets/src/main/java/org/apache/shindig/gadgets/uri/DefaultProxyUriManager.java
> (right):
> 
> http://codereview.appspot.com/1674041/diff/5001/6005#newcode122
>
java/gadgets/src/main/java/org/apache/shindig/gadgets/uri/DefaultProxyUriManager.java:122:
> uri.setScheme(config.getString(container, PROXY_SCHEME));
> No need to set schema, it is taken from the parent page

Copied AbsolutePathReferenceRewriter to
http://codereview.appspot.com/1699041/show

In this change, im trying to add the basic framework for domain balancing uri
which would take a list of uri's to domain balance and assign each uri a domain
assignment.
The actual domain assignment algo is not yet implemented as im still working on
it.

Please ignore AbsolutePathReferenceRewriter and AbsolutePathReferenceVisitor
files for now. They are needed to keep this patch running.

[gagan.goku, 2010-06-14 23:23:21]

http://codereview.appspot.com/1674041/diff/5001/6007
File
java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/DomainBalancingUriVisitor.java
(right):

http://codereview.appspot.com/1674041/diff/5001/6007#newcode44
java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/DomainBalancingUriVisitor.java:44:
public class DomainBalancingUriVisitor extends AbsolutePathReferenceVisitor {
On 2010/06/14 21:03:01, zhoresh wrote:
> Can you move this to separate review?
> At this point it doesn't really do much, so maybe explain what you are trying
to
> do here.

Moved out the Absolute Path rewriter change to
http://codereview.appspot.com/1699041/show

http://codereview.appspot.com/1674041/diff/5001/6007#newcode82
java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/DomainBalancingUriVisitor.java:82:
Map<Node, Uri> nodeToAbsoluteUri = new HashMap<Node, Uri>(nodes.size());
On 2010/06/14 21:03:01, zhoresh wrote:
> Maybe you can just use lists instead of maps. Unless I miss something, I think
> getRewrittenUris just go over the urls in order.
> 

But how will i know which node to change for the rewritten url to take effect ?
Are you suggesting something like a pair ?

http://codereview.appspot.com/1674041/diff/5001/6007#newcode142
java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/DomainBalancingUriVisitor.java:142:
* GGS or not. Uri's rewritten by other rewriters should not be rewritten as
On 2010/06/14 21:03:01, zhoresh wrote:
> please use Shindig instead of GGS

Done.

http://codereview.appspot.com/1674041/diff/5001/6007#newcode162
java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/DomainBalancingUriVisitor.java:162:
public void registerAccelHosts(List<String> accelHosts) {
On 2010/06/14 21:03:01, zhoresh wrote:
> I prefer injected constructor over injected set functions (and don't you also
> need to name the string list?)

Done.

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

http://codereview.appspot.com/1674041/diff/5001/6006#newcode56
java/gadgets/src/main/java/org/apache/shindig/gadgets/uri/AccelUriManager.java:56:
requestUri = new UriBuilder()
On 2010/06/14 21:03:01, zhoresh wrote:
> Can this be merged with RoxyUriManager? Basically you want similar code so it
> can hanle chain-param style the same way.
> 

I had actually started with proxyuri only, somehow got carried away :(

It looks lot simpler now.

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

http://codereview.appspot.com/1674041/diff/5001/6005#newcode122
java/gadgets/src/main/java/org/apache/shindig/gadgets/uri/DefaultProxyUriManager.java:122:
uri.setScheme(config.getString(container, PROXY_SCHEME));
On 2010/06/14 21:03:01, zhoresh wrote:
> No need to set schema, it is taken from the parent page

Done.

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

Please review.

[johnfargo, 2010-06-21 19:16:34]

Hi Gagan:

I'm a little confused still at the primary intended action of this CL. Is the
idea to balance domains that _aren't_ controlled by the Shindig server (ie. not
proxy URLs) but which _are_ known to be balance-able in some fashion? This type
of logic seems outside the scope of Shindig proper, and could be implemented in
other ways, eg. as a purpose-built ProxyUriManager injected into a given
installation.

Or is it instead intended as a way to simply ensure that when generating proxy
URIs, they're generated using some kind of balancing algorithm? In that case,
I'd still suggest that a ProxyUriManager implementation be used, but since the
URI is under control of Shindig, it would make sense to put in Shindig. An
example impl might, for instance, introduce a substitution token in proxy Uris:
"gadgets.uri.proxy.host": "%balanced%.foo.com"

...where the value of %balanced% could be supplied by an @Inject'ed class.

--j

[gagan.goku, 2010-06-22 14:51:09]

On 2010/06/21 19:16:34, johnfargo wrote:
> Hi Gagan:
> 
> I'm a little confused still at the primary intended action of this CL. Is the
> idea to balance domains that _aren't_ controlled by the Shindig server (ie.
not
> proxy URLs) but which _are_ known to be balance-able in some fashion? This
type
> of logic seems outside the scope of Shindig proper, and could be implemented
in
> other ways, eg. as a purpose-built ProxyUriManager injected into a given
> installation.
> 
> Or is it instead intended as a way to simply ensure that when generating proxy
> URIs, they're generated using some kind of balancing algorithm? In that case,
> I'd still suggest that a ProxyUriManager implementation be used, but since the
> URI is under control of Shindig, it would make sense to put in Shindig. An
> example impl might, for instance, introduce a substitution token in proxy
Uris:
> "gadgets.uri.proxy.host": "%balanced%.foo.com"
> 
> ...where the value of %balanced% could be supplied by an @Inject'ed class.
> 
> --j

Hi John

I agree that some kind of ProxyUriManager or DomainBalancingUriProvider might be
better here.
The original intent of this cl is to provide a generic enough rewriter that
allows for both the use cases you suggested (accelerated proxy urls + non
proxied urls that can be balanced).
I dont think your %balanced% solution is generic enough, but it looks really
clean :)

Could you elaborate more on how to provide its value using guice. Say the
current page has a list of uri U1, U2, U3 ...
and your domain balancing manager needs all the urls on page to come up with an
effective split.

Thanks
Gagan

[gagan.goku, 2010-06-22 14:51:19]

Please ignore changes in this file as being correctly modified as
http://codereview.appspot.com/1712043/diff/7002/32007

[gagan.goku, 2010-06-22 14:51:48]

On 2010/06/22 14:51:19, gagan.goku wrote:
> Please ignore changes in this file as being correctly modified as
> http://codereview.appspot.com/1712043/diff/7002/32007

I meant ignore changes in files AccelUriManager.java

[gagan.goku, 2010-06-22 19:40:34]

On 2010/06/22 14:51:48, gagan.goku wrote:
> On 2010/06/22 14:51:19, gagan.goku wrote:
> > Please ignore changes in this file as being correctly modified as
> > http://codereview.appspot.com/1712043/diff/7002/32007
> 
> I meant ignore changes in files AccelUriManager.java

I added DefaultProxyUriManager interface and ProxyingDomainBalancer as its basic
default implementation.
This allows for more advanced algorithms later.

[gagan.goku, 2010-06-24 01:40:47]

Please review.

[johnfargo, 2010-06-25 07:03:29]

Hey Gagan:

First, I think that this CL now includes a bunch of stuff that has since been
obviated, inc. all the Accel code and StringEncoding helper method.

For the Domain balancing work itself, I remain confused why we should inherit
from AbsoluteReferencePathVisitor. Domain balancing is a _property_ of a
ProxyUriManager, not a special rewriter.

The main rationale I could see in having a separate rewriter is to count the
number of links in the document to be balanced, and then to generate proxy Uris
for them all at once. Yet ProxyUriManager already does that - it gets the full
batch of Uris to proxy in the given List.

Given this, why not just write a DomainBalancingProxyUriManager? This class
needn't even implement its own "base" proxying algorithm. As noted in a previous
comment, you could simply @Inject into it a "base" ProxyUriManager, and then
have DomainBalancingProxyUriManager intelligently substitute a stable, balanced
prefix for the %balance% token in the resulting URI.

In this way, users can choose to opt into balancing or not simply through
configuration. Where an installation may have done:
bind(ProxyUriManager.class).to(MyProxyUriManager.class);

now they do:
bind(ProxyUriManager.class).annotatedWith(Names.named("shindig.proxy.balancing.base-manager")).to(MyProxyUriManager.class)
bind(ProxyUriManager.class).to(DomainBalancingProxyUriManager.class);

...and update config/container.js's gadgets.uri.proxy.host to include %balance%
somewhere in it.

Code could be as simple as:
class DomainBalancingProxyUriManager implements ProxyUriManager {
  @Inject
  DomainBalancingProxyUriManager(@Name("shindig.proxy.balancing.base-manager")
ProxyUriManager delegate) { ... }

  @Override
  public List<ProxyUri> make(List<Uri> uris...) {
    List<ProxyUri> proxied = delegate.make(uris...);
    // Modify the list here w/ your own balancing alg.
  }
}

Thoughts?

Best,
John

On 2010/06/24 01:40:47, gagan.goku wrote:
> Please review.

[gagan.goku, 2010-07-15 18:46:28]

Hi John

Addressed your comments. Please take another look.
For now, i have not bound ProxyUriManager to DomainBalancingProxyUriManager,
though i did test it with 0.localhost and 1.localhost and it seemed to be
generating the desired html.
If possible, I would like to take that up in the following change.

Thanks for the review.

[gagan.goku, 2010-08-28 02:49:59]

Closing this issue for now as we don't seem to be reaching a consensus.