Refactor the container configuration

Separate the code that holds and evaluates the container configurations from the code that reads them from disk.

Also provide for the possibility to change the container configuration after the fact. Add a configuration observer so other classes that hold data that depend on containers can update their state.

[jtarrio, 2010-10-07 00:26:09]

The patch is broken -- I'll reupload it shortly.

[jtarrio, 2010-10-07 00:44:43]

Hopefully fixed patch

[jtarrio, 2010-10-08 00:14:07]

I had missed a couple of classes I had to turn into observers too

[zhoresh, 2010-10-08 19:58:35]

Looks good, here are some preliminary comments, I am still looking

http://codereview.appspot.com/2350042/diff/14001/java/common/src/main/java/or...
File
java/common/src/main/java/org/apache/shindig/config/BasicContainerConfig.java
(right):

http://codereview.appspot.com/2350042/diff/14001/java/common/src/main/java/or...
java/common/src/main/java/org/apache/shindig/config/BasicContainerConfig.java:62:
public final int getInt(String container, String property) {
Does not need lock since it uses internally getProperties.
Same for few others that uses getProperty

http://codereview.appspot.com/2350042/diff/14001/java/common/src/main/java/or...
java/common/src/main/java/org/apache/shindig/config/BasicContainerConfig.java:205:
public String toString() {
Need read lock? Basically all instances that access config should lock

http://codereview.appspot.com/2350042/diff/14001/java/common/src/main/java/or...
java/common/src/main/java/org/apache/shindig/config/BasicContainerConfig.java:256:
} finally {
Add a catch and roll back of values if failed.
I actually would prefer to see setNewConfig work on temporary config, which then
if succeed replace the active config (you still need write lock to prevent other
writer rewrite at same time and ignore changes)

http://codereview.appspot.com/2350042/diff/14001/java/common/src/main/java/or...
java/common/src/main/java/org/apache/shindig/config/BasicContainerConfig.java:275:
protected void getOldConfig(Map<String, Map<String, Object>> newConfig) {
s/getOldConfig/putOldConfig

http://codereview.appspot.com/2350042/diff/14001/java/common/src/main/java/or...
java/common/src/main/java/org/apache/shindig/config/BasicContainerConfig.java:309:
Map<String, Object> base = config.get(container);
Maybe rename to "derived", base confused because I was thinking about base class

http://codereview.appspot.com/2350042/diff/14001/java/common/src/main/java/or...
java/common/src/main/java/org/apache/shindig/config/BasicContainerConfig.java:328:
* @param base The container to merge values from.
Names are a bit confusing, especially when you description call the first map
"base"

http://codereview.appspot.com/2350042/diff/14001/java/common/src/main/java/or...
java/common/src/main/java/org/apache/shindig/config/BasicContainerConfig.java:344:
if (update instanceof Map<?, ?> && existing instanceof Map<?, ?>) {
I a not sure I  see a reason for such recursive merge, wouldn't you want to just
replace content here?

[jtarrio, 2010-10-09 01:40:41]

http://codereview.appspot.com/2350042/diff/14001/java/common/src/main/java/or...
File
java/common/src/main/java/org/apache/shindig/config/BasicContainerConfig.java
(right):

http://codereview.appspot.com/2350042/diff/14001/java/common/src/main/java/or...
java/common/src/main/java/org/apache/shindig/config/BasicContainerConfig.java:62:
public final int getInt(String container, String property) {
On 2010/10/08 19:58:36, zhoresh wrote:
> Does not need lock since it uses internally getProperties.
> Same for few others that uses getProperty

Done.

http://codereview.appspot.com/2350042/diff/14001/java/common/src/main/java/or...
java/common/src/main/java/org/apache/shindig/config/BasicContainerConfig.java:205:
public String toString() {
On 2010/10/08 19:58:36, zhoresh wrote:
> Need read lock? Basically all instances that access config should lock

Done.

http://codereview.appspot.com/2350042/diff/14001/java/common/src/main/java/or...
java/common/src/main/java/org/apache/shindig/config/BasicContainerConfig.java:256:
} finally {
On 2010/10/08 19:58:36, zhoresh wrote:
> Add a catch and roll back of values if failed.
> I actually would prefer to see setNewConfig work on temporary config, which
then
> if succeed replace the active config (you still need write lock to prevent
other
> writer rewrite at same time and ignore changes)

Done.

http://codereview.appspot.com/2350042/diff/14001/java/common/src/main/java/or...
java/common/src/main/java/org/apache/shindig/config/BasicContainerConfig.java:275:
protected void getOldConfig(Map<String, Map<String, Object>> newConfig) {
On 2010/10/08 19:58:36, zhoresh wrote:
> s/getOldConfig/putOldConfig

This method doesn't exist anymore. The most similar one is the new
getTemporaryConfig

http://codereview.appspot.com/2350042/diff/14001/java/common/src/main/java/or...
java/common/src/main/java/org/apache/shindig/config/BasicContainerConfig.java:309:
Map<String, Object> base = config.get(container);
On 2010/10/08 19:58:36, zhoresh wrote:
> Maybe rename to "derived", base confused because I was thinking about base
class

Changed to a neutral "container" (and "container" to "name").

http://codereview.appspot.com/2350042/diff/14001/java/common/src/main/java/or...
java/common/src/main/java/org/apache/shindig/config/BasicContainerConfig.java:328:
* @param base The container to merge values from.
On 2010/10/08 19:58:36, zhoresh wrote:
> Names are a bit confusing, especially when you description call the first map
> "base"

Done.

http://codereview.appspot.com/2350042/diff/14001/java/common/src/main/java/or...
java/common/src/main/java/org/apache/shindig/config/BasicContainerConfig.java:344:
if (update instanceof Map<?, ?> && existing instanceof Map<?, ?>) {
On 2010/10/08 19:58:36, zhoresh wrote:
> I a not sure I  see a reason for such recursive merge, wouldn't you want to
just
> replace content here?

No, for maps (subproperties) we want to go inside and also merge recursively.
I've added a comment to mergeParents() and an unit test that checks for it.

[jtarrio, 2010-10-09 01:41:23]

Patch that addresses the issues in Ziv's comments

[gagan.goku, 2010-10-09 01:43:20]

+ cool-shindig-committers

On Sat, Oct 9, 2010 at 1:28 AM, <zhoresh@gmail.com> wrote:

> Looks good, here are some preliminary comments, I am still looking
>
>
>
>
http://codereview.appspot.com/2350042/diff/14001/java/common/src/main/java/or...
> File
>
> java/common/src/main/java/org/apache/shindig/config/BasicContainerConfig.java
> (right):
>
>
>
http://codereview.appspot.com/2350042/diff/14001/java/common/src/main/java/or...
>
>
java/common/src/main/java/org/apache/shindig/config/BasicContainerConfig.java:62:
> public final int getInt(String container, String property) {
> Does not need lock since it uses internally getProperties.
> Same for few others that uses getProperty
>
>
>
http://codereview.appspot.com/2350042/diff/14001/java/common/src/main/java/or...
>
>
java/common/src/main/java/org/apache/shindig/config/BasicContainerConfig.java:205:
> public String toString() {
> Need read lock? Basically all instances that access config should lock
>
>
>
http://codereview.appspot.com/2350042/diff/14001/java/common/src/main/java/or...
>
>
java/common/src/main/java/org/apache/shindig/config/BasicContainerConfig.java:256:
> } finally {
> Add a catch and roll back of values if failed.
> I actually would prefer to see setNewConfig work on temporary config,
> which then if succeed replace the active config (you still need write
> lock to prevent other writer rewrite at same time and ignore changes)
>
>
>
http://codereview.appspot.com/2350042/diff/14001/java/common/src/main/java/or...
>
>
java/common/src/main/java/org/apache/shindig/config/BasicContainerConfig.java:275:
> protected void getOldConfig(Map<String, Map<String, Object>> newConfig)
> {
> s/getOldConfig/putOldConfig
>
>
>
http://codereview.appspot.com/2350042/diff/14001/java/common/src/main/java/or...
>
>
java/common/src/main/java/org/apache/shindig/config/BasicContainerConfig.java:309:
> Map<String, Object> base = config.get(container);
> Maybe rename to "derived", base confused because I was thinking about
> base class
>
>
>
http://codereview.appspot.com/2350042/diff/14001/java/common/src/main/java/or...
>
>
java/common/src/main/java/org/apache/shindig/config/BasicContainerConfig.java:328:
> * @param base The container to merge values from.
> Names are a bit confusing, especially when you description call the
> first map "base"
>
>
>
http://codereview.appspot.com/2350042/diff/14001/java/common/src/main/java/or...
>
>
java/common/src/main/java/org/apache/shindig/config/BasicContainerConfig.java:344:
> if (update instanceof Map<?, ?> && existing instanceof Map<?, ?>) {
> I a not sure I  see a reason for such recursive merge, wouldn't you want
> to just replace content here?
>
> http://codereview.appspot.com/2350042/
>

[anupama.dutta, 2010-10-11 10:26:34]

http://codereview.appspot.com/2350042/diff/36001/java/gadgets/src/test/java/o...
File
java/gadgets/src/test/java/org/apache/shindig/gadgets/rewrite/StyleTagProxyEmbeddedUrlsVisitorTest.java
(right):

http://codereview.appspot.com/2350042/diff/36001/java/gadgets/src/test/java/o...
java/gadgets/src/test/java/org/apache/shindig/gadgets/rewrite/StyleTagProxyEmbeddedUrlsVisitorTest.java:73:
config.newTransaction().addContainer(MOCK_CONTAINER_CONFIG).commit();
Since this test was added to ensure that the container is taken into account
correctly (see http://codereview.appspot.com/2004042), shouldn't we add the
DEFAULT_CONTAINER_CONFIG as well?

[gagan.goku, 2010-10-11 12:34:43]

Hi Jacobo

Since this is a big change, it would be nice if you could mention your use case
here.

For example, if your goal is to be able to able to dynamically modify the
container configuration, then you could make a small code change to
JsonContainerConfig to expose a reload method, say something like:
    JsonContainerConfig.reloadConfig(String containers, Expressions expressions)
wherein you do all the work. You could make the getProperty method thread safe
and almost everything else should work as is.
Your current addition of transactions is a bit complex, hence makes one wonder
what the motivation is.

If your main use case is to separate out the code that loads config as strings
from the code that evaluates the expressions, then again you could get away with
smaller code changes to JsonContainerConfig.

In any case, i guess these 2 use cases can be split into smaller cls which would
make reviewing them easier.

Thanks
Gagan

[jtarrio, 2010-10-11 16:56:49]

 Hi, the motivation of the patch is to have the container configuration come
from several sources, some of which can change at run time and may not even
use JSON.

 The "ConfigObserver" stuff is there because several classes in Shindig are
initialized with the list of containers, so those have to be updated when
the list of available containers changes. The transactions are there because
I don't want to send 10 events if, say, 5 containers are added and 5
removed; just a big one with all 10 changes.

 The patch looks bigger than it actually is. As I'm not a committer, I
cannot use svn mv and svn cp because someone else needs to apply the patch
and commit for me, and the diff format cannot represent these types of
changes at all; therefore, every time I rename or split a class, it appears
as a deleted file plus one or more added files, instead of a renamed and
modified file.

On Mon, Oct 11, 2010 at 5:34 AM, <gagan.goku@gmail.com> wrote:

> Hi Jacobo
>
> Since this is a big change, it would be nice if you could mention your
> use case here.
>
> For example, if your goal is to be able to able to dynamically modify
> the container configuration, then you could make a small code change to
> JsonContainerConfig to expose a reload method, say something like:
>    JsonContainerConfig.reloadConfig(String containers, Expressions
> expressions)
> wherein you do all the work. You could make the getProperty method
> thread safe and almost everything else should work as is.
> Your current addition of transactions is a bit complex, hence makes one
> wonder what the motivation is.
>
> If your main use case is to separate out the code that loads config as
> strings from the code that evaluates the expressions, then again you
> could get away with smaller code changes to JsonContainerConfig.
>
> In any case, i guess these 2 use cases can be split into smaller cls
> which would make reviewing them easier.
>
> Thanks
> Gagan
>
>
> http://codereview.appspot.com/2350042/
>



-- 
   Jacobo Tarrío     |     http://jacobo.tarrio.org/

[jtarrio, 2010-10-11 17:01:56]

http://codereview.appspot.com/2350042/diff/36001/java/gadgets/src/test/java/o...
File
java/gadgets/src/test/java/org/apache/shindig/gadgets/rewrite/StyleTagProxyEmbeddedUrlsVisitorTest.java
(right):

http://codereview.appspot.com/2350042/diff/36001/java/gadgets/src/test/java/o...
java/gadgets/src/test/java/org/apache/shindig/gadgets/rewrite/StyleTagProxyEmbeddedUrlsVisitorTest.java:73:
config.newTransaction().addContainer(MOCK_CONTAINER_CONFIG).commit();
On 2010/10/11 10:26:34, anupama.dutta wrote:
> Since this test was added to ensure that the container is taken into account
> correctly (see http://codereview.appspot.com/2004042), shouldn't we add the
> DEFAULT_CONTAINER_CONFIG as well?

It is already there, in the configuration that is retrieved via
injector.getInstance.