[
https://issues.apache.org/jira/browse/SHINDIG-1647?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13132965#comment-13132965
]
[email protected] commented on SHINDIG-1647:
--------------------------------------------------------
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/2467/#review2753
-----------------------------------------------------------
Couple of small nits, but other than those the code looks ok to me. I still
have some general reservations on the strategy as a whole though which I'll
detail in the other review where this is actually used.
http://svn.apache.org/repos/asf/shindig/trunk/java/common/src/main/java/org/apache/shindig/config/ValueProvider.java
<https://reviews.apache.org/r/2467/#comment6193>
Extraneous semicolon
http://svn.apache.org/repos/asf/shindig/trunk/java/common/src/main/java/org/apache/shindig/config/ValueProvider.java
<https://reviews.apache.org/r/2467/#comment6195>
I'm guessing this is supposed to be:
cache.putIfAbsent(container, Maps.newConcurrentMap());
?
Or maybe this is the way it was intended which (I think) would make the
next line not needed?
I think I'd still do it in two lines either way though.
http://svn.apache.org/repos/asf/shindig/trunk/java/common/src/main/java/org/apache/shindig/config/ValueProvider.java
<https://reviews.apache.org/r/2467/#comment6194>
I think initialized might need to be declared as volatile for this
double-checked-locking to work properly.
Since this method is really only called when Shindig is initializing it
might be simpler to just synchronize the whole method.
- Jesse
On 2011-10-21 15:48:48, Stanton Sievers wrote:
bq.
bq. -----------------------------------------------------------
bq. This is an automatically generated e-mail. To reply, visit:
bq. https://reviews.apache.org/r/2467/
bq. -----------------------------------------------------------
bq.
bq. (Updated 2011-10-21 15:48:48)
bq.
bq.
bq. Review request for Ryan Baxter, Dan Dumont and Jesse Ciancetta.
bq.
bq.
bq. Summary
bq. -------
bq.
bq. This patch adds an abstraction layer between implementations and
configuration. The patch consists of an abstract class and some interfaces
that are used to read and observe ContainerConfig. Implementors of this class
could decide to read their configuration from ContainerConfig or provide values
from another source. This also allows code that needs to use configuration to
not have to worry about managing container.js keys. They can simply ask their
provider for values.
bq.
bq. I've separated this out from another review as it is generic. To see an
implementation of ValueProvider, you can look at this review:
https://reviews.apache.org/r/2362
bq.
bq.
bq. This addresses bug SHINDIG-1647.
bq. https://issues.apache.org/jira/browse/SHINDIG-1647
bq.
bq.
bq. Diffs
bq. -----
bq.
bq.
http://svn.apache.org/repos/asf/shindig/trunk/java/common/src/main/java/org/apache/shindig/config/ValueProvider.java
PRE-CREATION
bq.
bq. Diff: https://reviews.apache.org/r/2467/diff
bq.
bq.
bq. Testing
bq. -------
bq.
bq. No specific testing done on the patch, but testing was done in the other
review for those classes that implement and use it.
bq.
bq.
bq. Thanks,
bq.
bq. Stanton
bq.
bq.
> Provide an abstraction between implementations and the configurations they use
> ------------------------------------------------------------------------------
>
> Key: SHINDIG-1647
> URL: https://issues.apache.org/jira/browse/SHINDIG-1647
> Project: Shindig
> Issue Type: New Feature
> Components: Java
> Affects Versions: 3.0.0
> Reporter: Stanton Sievers
> Fix For: 3.0.0
>
>
> Add an abstraction layer between implementations and configuration. The
> patch consists of an abstract class and some interfaces that are used to read
> and observe ContainerConfig. Implementors of this class could decide to read
> their configuration from ContainerConfig or provide values from another
> source. This also allows code that needs to use configuration to not have to
> worry about managing container.js keys. They can simply ask their provider
> for values.
> I've separated this out from another review as it is generic. To see an
> implementation of ValueProvider, you can look at this review:
> https://reviews.apache.org/r/2362
--
This message is automatically generated by JIRA.
If you think it was sent incorrectly, please contact your JIRA administrators:
https://issues.apache.org/jira/secure/ContactAdministrators!default.jspa
For more information on JIRA, see: http://www.atlassian.com/software/jira
