You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@shindig.apache.org by zh...@gmail.com on 2010/10/08 21:58:36 UTC
Re: Refactor the container configuration (issue2350042)
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/
Re: Refactor the container configuration (issue2350042)
Posted by Gagandeep singh <ga...@gmail.com>.
+ cool-shindig-committers
On Sat, Oct 9, 2010 at 1:28 AM, <zh...@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/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/
>