Looks good, here are some preliminary comments, I am still looking
http://codereview.appspot.com/2350042/diff/14001/java/common/src/main/java/org/apache/shindig/config/BasicContainerConfig.java 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/org/apache/shindig/config/BasicContainerConfig.java#newcode62 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/org/apache/shindig/config/BasicContainerConfig.java#newcode205 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/org/apache/shindig/config/BasicContainerConfig.java#newcode256 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/org/apache/shindig/config/BasicContainerConfig.java#newcode275 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/org/apache/shindig/config/BasicContainerConfig.java#newcode309 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/org/apache/shindig/config/BasicContainerConfig.java#newcode328 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/org/apache/shindig/config/BasicContainerConfig.java#newcode344 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/
