[
https://issues.apache.org/jira/browse/SHINDIG-1647?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13134315#comment-13134315
]
[email protected] commented on SHINDIG-1647:
--------------------------------------------------------
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/2467/#review2796
-----------------------------------------------------------
Some conversation happened off of the review site for a while, so we'll be
replaying it here for the benefit of the dev community in the next few comments
:)
This conversation was actually on the related review:
https://reviews.apache.org/r/2362/ but the conversation applies more to this
one now that it has been moved to another review.
---
I'll respond to a few things from below.
bq. I still feel like this is really complicated and heavy-weight for doing
pretty
bq. much what ContainerConfig already does today.
It is, somewhat. Especially for the example we are committing. I admit it
does seem a bit over the top for something as simple as a key provider.
Keep in mind though, the new interface is so that we can inject it and swap out
implementations. Our key provider will behave differently.
bq. but everything else appears to be essentially just re-implementing a bunch
of
bq. the existing functionality.
One main point is that it's mostly all common code now (for the things that use
this pattern). Unless you are caching or deriving some value from the
ProvidedValue, there now no need for code to ever have to observe the container.
This provides a common structure so that others won't fall into concurrency
traps, and have duplicated code all over the place (there's tons of
ContainerConfig observer stuff all over shindig, some classes are independently
watching the same config settings)
bq. And we still have a full standalone interface (KeyProvider) and
implementation
bq. (KeyFileKeyProvider) for something that is only ever going to be used in
one
bq. place.
The interface is so that we can inject a different implementation, not so much
that we needed it to implement a ValueProvider. We wanted to maintain
back-compat.
bq. And it doesn't cleanly replace ContainerConfig -- so now classes that want
to
bq. use this strategy for configuration need to take ContainerConfig and an
bq. arbitrary configuration-type interface (in this case KeyProvider but in
other
bq. cases it would be different). I'm not saying I'd actually want to
abstract
bq. ContainerConfig away though -- I'm just pointing out that now we have to
bq. provide two configuration-providing components to those who want to use
this
bq. new strategy which seems sub-optimal to me.
I didn't want to replace ContainerConfig. The ValueProvider stuff uses
ContainerConfig. I think the thing I may not be explaining well is that I'm
not prociding a configuration, like "load your key from this file on start up".
I'm trying to provide an actual value (tried to be explicit in the name
ValueProvider instead of ConfigProvider). "This is the key to use, use it.".
That's why the strong typing. while you could use this to provide a value
straight out of the ContainerConfig, I'd argue that this impl lends itself well
to building more complex and well structures values that may consist of several
ContainerConfig settings.
bq. ContainerConfig is already in fairly wide usage throughout the codebase.
bq. If we wanted to reuse this new strategy in all the places ContainerConfig
is
bq. used we'd end up with something like 30ish more KeyProvider and
bq. KeyFileKeyProvider-like (but not exactly the same) interfaces and
bq. implementations.
That depends... There are places that observe the ContainerConfig to check
the value of a config setting the same way in multiple class files. This
approach lets you consolidate those into 1 injectable class, 1 instance and
less clutter with listeners for the container config.
For instance: the locked domain suffix config. This abstraction would make it
much easier to implement obtaining that value. No listeners are needed for the
classes that need it, they simply inject the provider and do a get on the
provided value. The value is updated in the provider automatically, and the
implementation for the provider is quite simple.
bq. Sorry for being the squeaky wheel on this set of changes.
Not at all! I'm glad to have such thorough reviews. Let me know if any of
this makes any sense. :)
- Dan
On 2011-10-24 16:28:55, 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-24 16:28:55)
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
