OSAPI Metadata-generation of apis

This patch allows the gadget server to rewrite the client js, passing in a list of services supported by the server, as specified in endpoints in the container config. 

This list of services is then used by osapi to generate client apis for the services provided by the server. 

For example, osapi.people, et.al, are no longer written statically as js, but are generated dynamically, based on the injected service list.

[bobevans, 2009-04-24 19:03:26]

This patch allows the gadget server to rewrite the client js, passing in a list
of services supported by the server, as specified in endpoints in the container
config. 

This list of services is then used by osapi to generate client apis for the
services provided by the server. 

For example, osapi.people, et.al, are no longer written statically as js, but
are generated dynamically, based on the injected service list.

// resending using remailer. Sorry for the duplication

[awiner, 2009-04-24 21:33:05]

http://codereview.appspot.com/50047/diff/1/38
File features/src/main/javascript/features/osapi/makerequest.js (right):

http://codereview.appspot.com/50047/diff/1/38#newcode23
Line 23: osapi.makeRequest = function() {
why not switch to osapi.http at this point?  We have the handler ready to go.

http://codereview.appspot.com/50047/diff/1/40
File features/src/main/javascript/features/osapi/osapi.js (right):

http://codereview.appspot.com/50047/diff/1/40#newcode23
Line 23: /**
clean up the indent of the above two

http://codereview.appspot.com/50047/diff/1/40#newcode38
Line 38: (function() {
we're already inside a closure:  why not move this function to just inside the
top closure, instead of creating it repeatedly?

e.g.

function createMethod(serviceName, methodName, endpointName) {
}

... which will also save the variable aliasing to work around closure behavior.

http://codereview.appspot.com/50047/diff/1/9
File
java/common/src/main/java/org/apache/shindig/protocol/RpcServiceFetcher.java
(right):

http://codereview.appspot.com/50047/diff/1/9#newcode62
Line 62: private ContainerConfig containerConfig;
final

http://codereview.appspot.com/50047/diff/1/9#newcode62
Line 62: private ContainerConfig containerConfig;
final

http://codereview.appspot.com/50047/diff/1/9#newcode80
Line 80: public Map<String, Set<String>> getServicesForContainer(String
container, String host) {
in practice, we don't need or want this to be host-relative.  For example,
locked domains puts every gadget on a different domain, but we don't want to
recompute services for each such gadget.  Store this once per-container, not
once per-container-and-host.

http://codereview.appspot.com/50047/diff/1/9#newcode85
Line 85: Map<String, Set<String>> endpointServices =
Maps.newHashMapWithExpectedSize(endpoints.size());
this should be a Multimap<String, String>, not a Map<String, Set<String>>

http://codereview.appspot.com/50047/diff/1/9#newcode87
Line 87: for (Iterator iterator = endpoints.iterator(); iterator.hasNext();) {
this can be a simple for loop.

http://codereview.appspot.com/50047/diff/1/9#newcode94
Line 94: @SuppressWarnings("unchecked")
move the @SuppressWarnings to the lines that need it instead of the whole
function

http://codereview.appspot.com/50047/diff/1/9#newcode98
Line 98: List<String> endpoints = (List<String>)
properties.get(OSAPI_BASE_ENDPOINTS);
take advantage of container config EL support:
  containerConfig.get("${Cur['gadgets.features'].endPoints}")

http://codereview.appspot.com/50047/diff/1/9#newcode100
Line 100: for (Iterator iterator = endpoints.iterator(); iterator.hasNext();) {
this could be a simple for (String endpoint : endpoints) loop

http://codereview.appspot.com/50047/diff/1/9#newcode110
Line 110: URL url = new URL(endpoint+"?method="+SYSTEM_LIST_METHODS_METHOD);
use Uri, not URL.  (Avoid java.net.URL like the plague, it is a mind-bogglingly
bad class.)

http://codereview.appspot.com/50047/diff/1/9#newcode110
Line 110: URL url = new URL(endpoint+"?method="+SYSTEM_LIST_METHODS_METHOD);
spaces around operators

http://codereview.appspot.com/50047/diff/1/9#newcode111
Line 111: HttpClient client = new HttpClient();
any particular reason not to just inject HttpFetcher (or RequestPipeline)?

http://codereview.appspot.com/50047/diff/1/9#newcode129
Line 129: private void getServicesFromJsonResponse(Set<String> services, String
content)
make this return a Set<String>, instead of taking one.  And should use
Collections.unmodifiableSet() or ImmutableSet.

http://codereview.appspot.com/50047/diff/1/8
File java/common/src/main/java/org/apache/shindig/protocol/RpcServiceLookup.java
(right):

http://codereview.appspot.com/50047/diff/1/8#newcode58
Line 58: private Map<String, Map<String, Set<String>>> containerServices;
all final

http://codereview.appspot.com/50047/diff/1/8#newcode79
Line 79: public synchronized Map<String, Set<String>> getServicesFor(String
container, String host) {
instead of synchronizing the methods, use concurrent hash maps (e.g. MapMaker
from Google Collections).

Also worth considering expiration of values, for two reasons:
- If there's an error loading the services, we'd like to be able to retry in the
near future
- Supports dynamically configured or updated servers (if a separate server
handles social requests, it can be updated with support for a new handler, and
we can detect that automatically)

http://codereview.appspot.com/50047/diff/1/8#newcode79
Line 79: public synchronized Map<String, Set<String>> getServicesFor(String
container, String host) {
Multimap would be a good choice here

http://codereview.appspot.com/50047/diff/1/8#newcode81
Line 81: if (container == null || container.length() == 0) {
seems like this should be Preconditions.checkArgument()

http://codereview.appspot.com/50047/diff/1/8#newcode86
Line 86: if (foundServices == null && !retrievedContainers.contains(container))
{
when would it be null, but retrieved?

http://codereview.appspot.com/50047/diff/1/8#newcode87
Line 87: foundServices = fetcher.getServicesForContainer(container, host);
we're fetching services for a container+host, but then caching purely on
container...

http://codereview.appspot.com/50047/diff/1/8#newcode89
Line 89: retrievedContainers.add(container);
instead of maintaining a list of retrievedContainers (just to distinguish null),
swap in an ImmutableMap.of() for the stored value

http://codereview.appspot.com/50047/diff/1/8#newcode91
Line 91: return (foundServices != null) ? foundServices : ImmutableMap.<String,
Set<String>>of();
would look simpler to say:

if (foundServices == null {
  foundServices = ImmutableMap.of();
}

return foundServices;

... since then you get generic types inferred

http://codereview.appspot.com/50047/diff/1/8#newcode99
Line 99: public synchronized void setServicesFor(String container, Map<String,
Set<String>> services) {
why is this public?

http://codereview.appspot.com/50047/diff/1/11
File
java/gadgets/src/main/java/org/apache/shindig/gadgets/render/RenderingGadgetRewriter.java
(right):

http://codereview.appspot.com/50047/diff/1/11#newcode390
Line 390: Collection<Feature> values = prefs.getFeatures().values();
values -> features

http://codereview.appspot.com/50047/diff/1/11#newcode398
Line 398: private void addOsapiSystemListMethodsConfig(String container,
Map<String, Object> config, String host) {
> 100 columns

http://codereview.appspot.com/50047/diff/1/11#newcode405
Line 405: private Map<String, Map<String, Map<String, String>>> mapEndpoints(
wow, Map of Maps of Maps.  Ouch.
Code needs some comments explaining what structure it's trying to build up.

FWIW, I think Map<String, Map<String, List<String>>> would be clearer - filling
in empty strings for the methods-to-be seems unnecessarily complex.

(Or at that point, Map<String, Multimap<String, String>>.  Muuuch simpler.)

http://codereview.appspot.com/50047/diff/1/11#newcode409
Line 409: for (Iterator<String> iterator = endpoints.keySet().iterator();
iterator.hasNext();) {
for (String endpoint : endpoints)

http://codereview.appspot.com/50047/diff/1/11#newcode412
Line 412: if (serviceMethods == null || serviceMethods.size() == 0) {
serviceMethods should never be null.
Also isEmpty() instead of size==0

http://codereview.appspot.com/50047/diff/1/11#newcode424
Line 424: for (Iterator<String> methodIter = methods.iterator();
methodIter.hasNext();) {
for (String method : methods)

[louiscryan, 2009-04-24 22:01:25]

First batch. I need to noodle on the bindig/dispatch JS code some more.

http://codereview.appspot.com/50047/diff/1/43
File features/src/main/javascript/features/osapi/batch.js (right):

http://codereview.appspot.com/50047/diff/1/43#newcode1
Line 1: /*
In general this code needs more thorough documentation.

http://codereview.appspot.com/50047/diff/1/43#newcode24
Line 24: * Currently, since makeRequest is proxied, these requests are handled
separately to
is this comment valid? makeRequest is'nt part of the api.

http://codereview.appspot.com/50047/diff/1/43#newcode30
Line 30: * Latch that waits for the batch of json requests and each makeRequest
call to finish before
comment cleanup for makeRequest, switch to osapi.http. Probably best to make
generic and describe how calls are partitioned across end-points.

http://codereview.appspot.com/50047/diff/1/43#newcode70
Line 70: finishedJsonRequest : finishNonProxiedRequest,
are both of these needed with the transition to osapi.http

http://codereview.appspot.com/50047/diff/1/43#newcode95
Line 95: var executeJsonRequests = function(jsonRequests, json,
getDataFromResult, countDownLatch) {
parameter documentation would be helpful

http://codereview.appspot.com/50047/diff/1/43#newcode111
Line 111: var executeProxiedRequests = function(proxiedRequests, countDownLatch)
{
not needed with elimination of makeRequest?

http://codereview.appspot.com/50047/diff/1/43#newcode144
Line 144: if (request.isMakeRequest) {
remove makeRequest specific piece.

http://codereview.appspot.com/50047/diff/1/44
File features/src/main/javascript/features/osapi/jsonrequest.js (right):

http://codereview.appspot.com/50047/diff/1/44#newcode22
Line 22: rpcUrl: The url of the opensocial data endpoint.
opensocial -> JSON-RPC

http://codereview.appspot.com/50047/diff/1/44#newcode30
Line 30: * The bare mechanism for creating json opensocial requests.
opensocial -> JSON-RPC

http://codereview.appspot.com/50047/diff/1/44#newcode85
Line 85: //    for (var i = 0; i < result.data.length; i++) {
remove if unused

http://codereview.appspot.com/50047/diff/1/44#newcode101
Line 101: var defaultUserId = '@viewer';
remove?

http://codereview.appspot.com/50047/diff/1/44#newcode126
Line 126: var ensureUserId = function(options) {
remove?

http://codereview.appspot.com/50047/diff/1/44#newcode135
Line 135: var ensureGroupId = function(options) {
remove?

http://codereview.appspot.com/50047/diff/1/44#newcode143
Line 143: ensureUserId(params);
remove?

http://codereview.appspot.com/50047/diff/1/44#newcode144
Line 144: ensureGroupId(params);
remove?

http://codereview.appspot.com/50047/diff/1/38
File features/src/main/javascript/features/osapi/makerequest.js (right):

http://codereview.appspot.com/50047/diff/1/38#newcode23
Line 23: osapi.makeRequest = function() {
remove?

http://codereview.appspot.com/50047/diff/1/40
File features/src/main/javascript/features/osapi/osapi.js (right):

http://codereview.appspot.com/50047/diff/1/40#newcode53
Line 53: var requiredConfig = {
not sure this serves a purpose? its hidden in a closure

http://codereview.appspot.com/50047/diff/1/39
File features/src/main/javascript/features/osapi/peoplehelpers.js (right):

http://codereview.appspot.com/50047/diff/1/39#newcode20
Line 20: osapi.people = osapi.people || {};
shouldnt do this, code should be loaded after init and if osapi.people doesnt
exist neither do the helpers.

http://codereview.appspot.com/50047/diff/1/39#newcode50
Line 50: options.userId = "@viewer";
technically you dont need to set these as they are the protocol defaults.

http://codereview.appspot.com/50047/diff/1/41
File features/src/main/javascript/features/osapi/util.js (right):

http://codereview.appspot.com/50047/diff/1/41#newcode40
Line 40: if (httpError == "Error 501") {
might be better to express as a map

http://codereview.appspot.com/50047/diff/1/18
File features/src/test/javascript/features/osapi/activitiestest.js (right):

http://codereview.appspot.com/50047/diff/1/18#newcode36
Line 36: "system" : { "listMethods" : "" },
this seems weird. can we use arrays here instead.

http://codereview.appspot.com/50047/diff/1/9
File
java/common/src/main/java/org/apache/shindig/protocol/RpcServiceFetcher.java
(right):

http://codereview.appspot.com/50047/diff/1/9#newcode91
Line 91: return endpointServices;
make immutable before returning

http://codereview.appspot.com/50047/diff/1/9#newcode110
Line 110: URL url = new URL(endpoint+"?method="+SYSTEM_LIST_METHODS_METHOD);
Any reason not to bind and use the HttpFetcher?

http://codereview.appspot.com/50047/diff/1/3
File java/server/src/test/resources/endtoend/osapi/activitiesTest.xml (right):

http://codereview.appspot.com/50047/diff/1/3#newcode37
Line 37: osapi.activities.get({ userId : 'canonical', groupId :
'@self'}).execute(receivedData);
groupid not needed.

[bobevans, 2009-04-29 19:55:04]

left out the el and httpfetcher suggestions for a later date given the magnitude
of changes already introduced.

http://codereview.appspot.com/50047/diff/1/43
File features/src/main/javascript/features/osapi/batch.js (right):

http://codereview.appspot.com/50047/diff/1/43#newcode1
Line 1: /*
On 2009/04/24 22:01:26, louiscryan wrote:
> In general this code needs more thorough documentation.

Done.

http://codereview.appspot.com/50047/diff/1/43#newcode24
Line 24: * Currently, since makeRequest is proxied, these requests are handled
separately to
On 2009/04/24 22:01:26, louiscryan wrote:
> is this comment valid? makeRequest is'nt part of the api.

removed.

http://codereview.appspot.com/50047/diff/1/43#newcode30
Line 30: * Latch that waits for the batch of json requests and each makeRequest
call to finish before
On 2009/04/24 22:01:26, louiscryan wrote:
> comment cleanup for makeRequest, switch to osapi.http. Probably best to make
> generic and describe how calls are partitioned across end-points.

deleted all makeRequest code.

http://codereview.appspot.com/50047/diff/1/43#newcode70
Line 70: finishedJsonRequest : finishNonProxiedRequest,
On 2009/04/24 22:01:26, louiscryan wrote:
> are both of these needed with the transition to osapi.http

Done.

http://codereview.appspot.com/50047/diff/1/43#newcode95
Line 95: var executeJsonRequests = function(jsonRequests, json,
getDataFromResult, countDownLatch) {
On 2009/04/24 22:01:26, louiscryan wrote:
> parameter documentation would be helpful

Done.

http://codereview.appspot.com/50047/diff/1/43#newcode111
Line 111: var executeProxiedRequests = function(proxiedRequests, countDownLatch)
{
On 2009/04/24 22:01:26, louiscryan wrote:
> not needed with elimination of makeRequest?

Done.

http://codereview.appspot.com/50047/diff/1/43#newcode144
Line 144: if (request.isMakeRequest) {
On 2009/04/24 22:01:26, louiscryan wrote:
> remove makeRequest specific piece.

Done.

http://codereview.appspot.com/50047/diff/1/38
File features/src/main/javascript/features/osapi/makerequest.js (right):

http://codereview.appspot.com/50047/diff/1/38#newcode23
Line 23: osapi.makeRequest = function() {
On 2009/04/24 22:01:26, louiscryan wrote:
> remove?

removed all makeRequest code since it is being replaced by osapi.http json rpc
handler

http://codereview.appspot.com/50047/diff/1/40
File features/src/main/javascript/features/osapi/osapi.js (right):

http://codereview.appspot.com/50047/diff/1/40#newcode23
Line 23: /**
On 2009/04/24 21:33:05, awiner wrote:
> clean up the indent of the above two

Done.

http://codereview.appspot.com/50047/diff/1/40#newcode38
Line 38: (function() {
On 2009/04/24 21:33:05, awiner wrote:
> we're already inside a closure:  why not move this function to just inside the
> top closure, instead of creating it repeatedly?
> 
> e.g.
> 
> function createMethod(serviceName, methodName, endpointName) {
> }
> 
> ... which will also save the variable aliasing to work around closure
behavior.
> 

Done.

http://codereview.appspot.com/50047/diff/1/40#newcode53
Line 53: var requiredConfig = {
On 2009/04/24 22:01:26, louiscryan wrote:
> not sure this serves a purpose? its hidden in a closure

it's just a necessary parameter to the register method underneath.

Inlined a literal instead.

http://codereview.appspot.com/50047/diff/1/9
File
java/common/src/main/java/org/apache/shindig/protocol/RpcServiceFetcher.java
(right):

http://codereview.appspot.com/50047/diff/1/9#newcode62
Line 62: private ContainerConfig containerConfig;
On 2009/04/24 21:33:05, awiner wrote:
> final

Done.

http://codereview.appspot.com/50047/diff/1/9#newcode80
Line 80: public Map<String, Set<String>> getServicesForContainer(String
container, String host) {
On 2009/04/24 21:33:05, awiner wrote:
> in practice, we don't need or want this to be host-relative.  For example,
> locked domains puts every gadget on a different domain, but we don't want to
> recompute services for each such gadget.  Store this once per-container, not
> once per-container-and-host.

This is a problematic tradeoff. 
I see the side you mention, even though locked domain is in actuality bounded to
a pretty low number of domains right?

The other side of this is that the endpoint is needed per host to actually make
the rpc call, so it's quite convenient to store the service methods under the
actual endpoint that's going to be called for the purpose of then constructing
the rpc calls in the client, and also this is necessary for breaking up the
calls in a batch into the distinct endpoints. We don't have any good way to
distinguish a unique host at the moment, other than that the container specifies
the endpoints which reflect different hosts. I'll think about this some more, it
seems solvable without much difficulty, but I'd like to get this patch done,
then come back and do that rework since it stripes through end-to-end. 

I think one solution would be to
1) Store only by container, with parameterized endpoints (as they are in the
container config).
2) Expand the host to make the actual calls to retrieve the services.
3) attach the endpoints as a property of the service method structure which gets
stored.
4) re-do the substitution on the client for a given endpoint
5) use the property for distinguishing the batching of calls to the same
endpoint (similar to the way its done now).

http://codereview.appspot.com/50047/diff/1/9#newcode85
Line 85: Map<String, Set<String>> endpointServices =
Maps.newHashMapWithExpectedSize(endpoints.size());
On 2009/04/24 21:33:05, awiner wrote:
> this should be a Multimap<String, String>, not a Map<String, Set<String>>

Done.

http://codereview.appspot.com/50047/diff/1/9#newcode87
Line 87: for (Iterator iterator = endpoints.iterator(); iterator.hasNext();) {
On 2009/04/24 21:33:05, awiner wrote:
> this can be a simple for loop.

Done.

http://codereview.appspot.com/50047/diff/1/9#newcode91
Line 91: return endpointServices;
On 2009/04/24 22:01:26, louiscryan wrote:
> make immutable before returning

Done.

http://codereview.appspot.com/50047/diff/1/9#newcode94
Line 94: @SuppressWarnings("unchecked")
On 2009/04/24 21:33:05, awiner wrote:
> move the @SuppressWarnings to the lines that need it instead of the whole
> function

Done.

http://codereview.appspot.com/50047/diff/1/9#newcode100
Line 100: for (Iterator iterator = endpoints.iterator(); iterator.hasNext();) {
On 2009/04/24 21:33:05, awiner wrote:
> this could be a simple for (String endpoint : endpoints) loop

Done.

http://codereview.appspot.com/50047/diff/1/9#newcode110
Line 110: URL url = new URL(endpoint+"?method="+SYSTEM_LIST_METHODS_METHOD);
On 2009/04/24 21:33:05, awiner wrote:
> spaces around operators

Done.

http://codereview.appspot.com/50047/diff/1/9#newcode110
Line 110: URL url = new URL(endpoint+"?method="+SYSTEM_LIST_METHODS_METHOD);
On 2009/04/24 21:33:05, awiner wrote:
> spaces around operators

Done.

http://codereview.appspot.com/50047/diff/1/9#newcode110
Line 110: URL url = new URL(endpoint+"?method="+SYSTEM_LIST_METHODS_METHOD);
On 2009/04/24 21:33:05, awiner wrote:
> spaces around operators

Done.

http://codereview.appspot.com/50047/diff/1/9#newcode110
Line 110: URL url = new URL(endpoint+"?method="+SYSTEM_LIST_METHODS_METHOD);
On 2009/04/24 21:33:05, awiner wrote:
> use Uri, not URL.  (Avoid java.net.URL like the plague, it is a
mind-bogglingly
> bad class.)

Done.

http://codereview.appspot.com/50047/diff/1/9#newcode111
Line 111: HttpClient client = new HttpClient();
On 2009/04/24 21:33:05, awiner wrote:
> any particular reason not to just inject HttpFetcher (or RequestPipeline)?

No good reason, I just didn't know about it. Is it worth changing?

http://codereview.appspot.com/50047/diff/1/9#newcode129
Line 129: private void getServicesFromJsonResponse(Set<String> services, String
content)
On 2009/04/24 21:33:05, awiner wrote:
> make this return a Set<String>, instead of taking one.  And should use
> Collections.unmodifiableSet() or ImmutableSet.

Done.

http://codereview.appspot.com/50047/diff/1/9#newcode129
Line 129: private void getServicesFromJsonResponse(Set<String> services, String
content)
On 2009/04/24 21:33:05, awiner wrote:
> make this return a Set<String>, instead of taking one.  And should use
> Collections.unmodifiableSet() or ImmutableSet.

Done.

http://codereview.appspot.com/50047/diff/1/8
File java/common/src/main/java/org/apache/shindig/protocol/RpcServiceLookup.java
(right):

http://codereview.appspot.com/50047/diff/1/8#newcode58
Line 58: private Map<String, Map<String, Set<String>>> containerServices;
On 2009/04/24 21:33:05, awiner wrote:
> all final

Done.

http://codereview.appspot.com/50047/diff/1/8#newcode79
Line 79: public synchronized Map<String, Set<String>> getServicesFor(String
container, String host) {
On 2009/04/24 21:33:05, awiner wrote:
> Multimap would be a good choice here

Done.

http://codereview.appspot.com/50047/diff/1/8#newcode81
Line 81: if (container == null || container.length() == 0) {
On 2009/04/24 21:33:05, awiner wrote:
> seems like this should be Preconditions.checkArgument()

Done.

http://codereview.appspot.com/50047/diff/1/8#newcode86
Line 86: if (foundServices == null && !retrievedContainers.contains(container))
{
On 2009/04/24 21:33:05, awiner wrote:
> when would it be null, but retrieved?

Reworked all this to use an expiration concurrent hash, with a guiced in
timeout, so that the server endpoints will be re-checked periodically

http://codereview.appspot.com/50047/diff/1/8#newcode89
Line 89: retrievedContainers.add(container);
On 2009/04/24 21:33:05, awiner wrote:
> instead of maintaining a list of retrievedContainers (just to distinguish
null),
> swap in an ImmutableMap.of() for the stored value

Done.

http://codereview.appspot.com/50047/diff/1/8#newcode91
Line 91: return (foundServices != null) ? foundServices : ImmutableMap.<String,
Set<String>>of();
On 2009/04/24 21:33:05, awiner wrote:
> would look simpler to say:
> 
> if (foundServices == null {
>   foundServices = ImmutableMap.of();
> }
> 
> return foundServices;
> 
> ... since then you get generic types inferred
Done. But gross edge case of generics.

http://codereview.appspot.com/50047/diff/1/8#newcode99
Line 99: public synchronized void setServicesFor(String container, Map<String,
Set<String>> services) {
On 2009/04/24 21:33:05, awiner wrote:
> why is this public?

Made package visible for testing.

http://codereview.appspot.com/50047/diff/1/11
File
java/gadgets/src/main/java/org/apache/shindig/gadgets/render/RenderingGadgetRewriter.java
(right):

http://codereview.appspot.com/50047/diff/1/11#newcode390
Line 390: Collection<Feature> values = prefs.getFeatures().values();
On 2009/04/24 21:33:05, awiner wrote:
> values -> features

Done.

http://codereview.appspot.com/50047/diff/1/11#newcode398
Line 398: private void addOsapiSystemListMethodsConfig(String container,
Map<String, Object> config, String host) {
On 2009/04/24 21:33:05, awiner wrote:
> > 100 columns

Done.

http://codereview.appspot.com/50047/diff/1/11#newcode405
Line 405: private Map<String, Map<String, Map<String, String>>> mapEndpoints(
On 2009/04/24 21:33:05, awiner wrote:
> wow, Map of Maps of Maps.  Ouch.
> Code needs some comments explaining what structure it's trying to build up.
> 
> FWIW, I think Map<String, Map<String, List<String>>> would be clearer -
filling
> in empty strings for the methods-to-be seems unnecessarily complex.
> 
> (Or at that point, Map<String, Multimap<String, String>>.  Muuuch simpler.)

Yeah, basically I was following the examples for building up json structures in
the other methods here. Reworked all of this to be much simpler and more
reasonable.

http://codereview.appspot.com/50047/diff/1/11#newcode409
Line 409: for (Iterator<String> iterator = endpoints.keySet().iterator();
iterator.hasNext();) {
On 2009/04/24 21:33:05, awiner wrote:
> for (String endpoint : endpoints) 

Done.

http://codereview.appspot.com/50047/diff/1/11#newcode412
Line 412: if (serviceMethods == null || serviceMethods.size() == 0) {
On 2009/04/24 21:33:05, awiner wrote:
> serviceMethods should never be null.
> Also isEmpty() instead of size==0

Done.

http://codereview.appspot.com/50047/diff/1/11#newcode424
Line 424: for (Iterator<String> methodIter = methods.iterator();
methodIter.hasNext();) {
On 2009/04/24 21:33:05, awiner wrote:
> for (String method : methods)

Done.

[bobevans, 2009-04-29 20:23:24]

with feedback changes

[louiscryan, 2009-04-29 20:57:14]

Much better. Still need to cleanup the per-endpoint caching of services and use
of HttpClient

http://codereview.appspot.com/50047/diff/1/9
File
java/common/src/main/java/org/apache/shindig/protocol/RpcServiceFetcher.java
(right):

http://codereview.appspot.com/50047/diff/1/9#newcode80
Line 80: public Map<String, Set<String>> getServicesForContainer(String
container, String host) {
The steps you outline for fixing this seem correct. Performing the substitution
client-side is pretty straightforward and is done for other services. I'd prefer
to see that done in this checkin.

On 2009/04/29 19:55:04, bobevans wrote:
> On 2009/04/24 21:33:05, awiner wrote:
> > in practice, we don't need or want this to be host-relative.  For example,
> > locked domains puts every gadget on a different domain, but we don't want to
> > recompute services for each such gadget.  Store this once per-container, not
> > once per-container-and-host.
> 
> This is a problematic tradeoff. 
> I see the side you mention, even though locked domain is in actuality bounded
to
> a pretty low number of domains right?
> 
> The other side of this is that the endpoint is needed per host to actually
make
> the rpc call, so it's quite convenient to store the service methods under the
> actual endpoint that's going to be called for the purpose of then constructing
> the rpc calls in the client, and also this is necessary for breaking up the
> calls in a batch into the distinct endpoints. We don't have any good way to
> distinguish a unique host at the moment, other than that the container
specifies
> the endpoints which reflect different hosts. I'll think about this some more,
it
> seems solvable without much difficulty, but I'd like to get this patch done,
> then come back and do that rework since it stripes through end-to-end. 
> 
> I think one solution would be to
> 1) Store only by container, with parameterized endpoints (as they are in the
> container config).
> 2) Expand the host to make the actual calls to retrieve the services.
> 3) attach the endpoints as a property of the service method structure which
gets
> stored.
> 4) re-do the substitution on the client for a given endpoint
> 5) use the property for distinguishing the batching of calls to the same
> endpoint (similar to the way its done now).

http://codereview.appspot.com/50047/diff/1/9#newcode111
Line 111: HttpClient client = new HttpClient();
On 2009/04/29 19:55:04, bobevans wrote:
> On 2009/04/24 21:33:05, awiner wrote:
> > any particular reason not to just inject HttpFetcher (or RequestPipeline)?
> 
> No good reason, I just didn't know about it. Is it worth changing?

Yes, definitely worth changing