+ cool-shindig-committers On Sat, Oct 9, 2010 at 1:28 AM, <[email protected]> wrote:
> 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/ >
