Add data pipelining support to Shindig

Implementing a prototype of data pipelining in Shindig.  The most recent patch set is missing:
 - support for <os:DataRequest>

It also requires sign_owner and/or sign_viewer if the requested data refers to owner and/or viewer.

This is https://issues.apache.org/jira/browse/SHINDIG-696

[awiner, 2008-11-13 00:59:58]

Updated patch

[louiscryan, 2008-11-14 22:39:22]

Looks awesome. Happy to commit

http://codereview.appspot.com/8648/diff/28/223
File config/container.js (right):

http://codereview.appspot.com/8648/diff/28/223#newcode78
Line 78: "gadgets.osDataUri" : "http://%host%/social/rpc",
Add a comment explaining what this is used for

http://codereview.appspot.com/8648/diff/28/220
File java/gadgets/src/main/java/org/apache/shindig/gadgets/spec/SocialData.java
(right):

http://codereview.appspot.com/8648/diff/28/220#newcode50
Line 50: for (Node node = element.getFirstChild(); node != null; node =
node.getNextSibling()) {
consider migrating this filtering check into XmlUtil function

getChildrenWithNamespace

http://codereview.appspot.com/8648/diff/28/221
File java/gadgets/src/main/java/org/apache/shindig/gadgets/spec/View.java
(right):

http://codereview.appspot.com/8648/diff/28/221#newcode164
Line 164: // TODO: support hangman substitutions for social preloads?
over my dead body

[awiner, 2008-11-16 00:17:26]

Uploaded new patch, including proper caching support, some tweaks for numeric
attributes, and a bunch of unit tests.

http://codereview.appspot.com/8648/diff/28/223
File config/container.js (right):

http://codereview.appspot.com/8648/diff/28/223#newcode78
Line 78: "gadgets.osDataUri" : "http://%host%/social/rpc",
On 2008/11/14 22:39:22, louiscryan wrote:
> Add a comment explaining what this is used for

Done.

http://codereview.appspot.com/8648/diff/28/220
File java/gadgets/src/main/java/org/apache/shindig/gadgets/spec/SocialData.java
(right):

http://codereview.appspot.com/8648/diff/28/220#newcode50
Line 50: for (Node node = element.getFirstChild(); node != null; node =
node.getNextSibling()) {
On 2008/11/14 22:39:22, louiscryan wrote:
> consider migrating this filtering check into XmlUtil function
> 
> getChildrenWithNamespace

A good idea - added TODO.

[awiner, 2008-11-16 00:18:13]

Added unit tests, caching support, etc.

[awiner, 2008-12-08 23:37:51]

Added os:MakeRequest/ViewerRequest/OwnerRequest, fixed namespace, separated
handling of <Preload>

[etnu00, 2008-12-09 18:36:18]

http://codereview.appspot.com/8648/diff/401/426
File java/common/src/main/java/org/apache/shindig/common/xml/XmlUtil.java
(right):

http://codereview.appspot.com/8648/diff/401/426#newcode63
Line 63: builderFactory.setNamespaceAware(true);
Is there any reason why this isn't in the existing static block that configures
the factory?

http://codereview.appspot.com/8648/diff/401/417
File
java/gadgets/src/main/java/org/apache/shindig/gadgets/preload/ConcurrentPreloads.java
(right):

http://codereview.appspot.com/8648/diff/401/417#newcode93
Line 93: } catch (ExecutionException e) {
This winds up swallowing any errors that happen, which seems very wrong,
especially if I get multiple errors in a given call. How is the developer
supposed to know why something didn't work? What should we do when this happens
for something that gets posted?

Now that I think about it, I don't think this has to be changed to a
Collection<Callable<...>>. If I have a Preloader that needs to fire off a single
request to satisfy multiple data items, why can't I just use a barrier in the
subsets of the call and have them all blocking on the same loader?

http://codereview.appspot.com/8648/diff/401/418
File
java/gadgets/src/main/java/org/apache/shindig/gadgets/preload/HttpPreloader.java
(right):

http://codereview.appspot.com/8648/diff/401/418#newcode78
Line 78: static public HttpRequest newHttpRequest(GadgetContext context,
public static

http://codereview.appspot.com/8648/diff/401/425
File java/gadgets/src/main/java/org/apache/shindig/gadgets/spec/View.java
(right):

http://codereview.appspot.com/8648/diff/401/425#newcode2
Line 2: * Licensed to the Apache Software Foundation (ASF) under one
Appspot screws this review up.

Why are the social preloads exported as Object instead of their base type?

We really need to get unification between the Preload and os:MakeRequest.

[etnu00, 2008-12-09 18:38:12]

http://codereview.appspot.com/8648/diff/28/221
File java/gadgets/src/main/java/org/apache/shindig/gadgets/spec/View.java
(right):

http://codereview.appspot.com/8648/diff/28/221#newcode164
Line 164: // TODO: support hangman substitutions for social preloads?
On 2008/11/14 22:39:22, louiscryan wrote:
> over my dead body

Without that, there's absolutely no way to do localization. I suppose if we only
want developers targeting a single language that might be acceptable.

[awiner, 2008-12-09 19:26:48]

http://codereview.appspot.com/8648/diff/28/221
File java/gadgets/src/main/java/org/apache/shindig/gadgets/spec/View.java
(right):

http://codereview.appspot.com/8648/diff/28/221#newcode164
Line 164: // TODO: support hangman substitutions for social preloads?
On 2008/12/09 18:38:12, etnu00 wrote:
> On 2008/11/14 22:39:22, louiscryan wrote:
> > over my dead body
> 
> Without that, there's absolutely no way to do localization. I suppose if we
only
> want developers targeting a single language that might be acceptable.

There is a way - EL support, which already (will) give access to user prefs.  Do
we want two different ways to do the same thing within these elements?

http://codereview.appspot.com/8648/diff/401/426
File java/common/src/main/java/org/apache/shindig/common/xml/XmlUtil.java
(right):

http://codereview.appspot.com/8648/diff/401/426#newcode63
Line 63: builderFactory.setNamespaceAware(true);
On 2008/12/09 18:36:19, etnu00 wrote:
> Is there any reason why this isn't in the existing static block that
configures
> the factory?

I'm blind.  Moved.

http://codereview.appspot.com/8648/diff/401/417
File
java/gadgets/src/main/java/org/apache/shindig/gadgets/preload/ConcurrentPreloads.java
(right):

http://codereview.appspot.com/8648/diff/401/417#newcode93
Line 93: } catch (ExecutionException e) {
On 2008/12/09 18:36:19, etnu00 wrote:
> This winds up swallowing any errors that happen, which seems very wrong,
> especially if I get multiple errors in a given call. How is the developer
> supposed to know why something didn't work? What should we do when this
happens
> for something that gets posted?
> 
> Now that I think about it, I don't think this has to be changed to a
> Collection<Callable<...>>. If I have a Preloader that needs to fire off a
single
> request to satisfy multiple data items, why can't I just use a barrier in the
> subsets of the call and have them all blocking on the same loader?

I've changed the API from Collection<Callable<Map<String, PreloadedData>> to
Collection<Callable<PreloadedData>>, where PreloadedData supports Map<String,
Object> instead of just Object, and PreloadedData.toJson() is what can throw an
exception.  The exception isn't swallowed.

I thought about the single-barrier-for-multiple-data-items approach, but so far
think it's more complex than where I am (after the above change).  Happy to
further revisit.

http://codereview.appspot.com/8648/diff/401/418
File
java/gadgets/src/main/java/org/apache/shindig/gadgets/preload/HttpPreloader.java
(right):

http://codereview.appspot.com/8648/diff/401/418#newcode78
Line 78: static public HttpRequest newHttpRequest(GadgetContext context,
On 2008/12/09 18:36:19, etnu00 wrote:
> public static

Done.

http://codereview.appspot.com/8648/diff/401/425
File java/gadgets/src/main/java/org/apache/shindig/gadgets/spec/View.java
(right):

http://codereview.appspot.com/8648/diff/401/425#newcode2
Line 2: * Licensed to the Apache Software Foundation (ASF) under one
On 2008/12/09 18:36:19, etnu00 wrote:
> Appspot screws this review up.
> 
> Why are the social preloads exported as Object instead of their base type?

Base type meaning SocialData?  The code's merging across multiple views, but
that's pointless, since proxied rendering can only have one view section.  I can
clean this up.

> We really need to get unification between the Preload and os:MakeRequest.

Amen, but I'd like that to be a follow-up change, after spec discussions about
what the JSON envelope should look like for os:MakeRequest.

[awiner, 2008-12-10 22:16:16]

Addressed (most of) Kevin's review, and resolved ongoing bitrot.

[etnu00, 2008-12-10 23:29:15]

Looks good to me. Some last few comments on naming / style.

http://codereview.appspot.com/8648/diff/801/453
File
java/gadgets/src/main/java/org/apache/shindig/gadgets/preload/ConcurrentPreloaderService.java
(right):

http://codereview.appspot.com/8648/diff/801/453#newcode71
Line 71: // is counter-productive
It doesn't really matter though since the retrieval of the preloaded data blocks
until everything is complete. More wiring has to be done to make that behavior
work.

http://codereview.appspot.com/8648/diff/801/465
File java/gadgets/src/main/java/org/apache/shindig/gadgets/spec/SocialData.java
(right):

http://codereview.appspot.com/8648/diff/801/465#newcode40
Line 40: public class SocialData {
I'd suggest calling this PipelinedData or something along those lines. Not
everything in data pipelining is necessarily "social", including...

http://codereview.appspot.com/8648/diff/801/465#newcode43
Line 43: private final Map<String, Preload> httpPreloads;
this.