You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@shindig.apache.org by "Stanton Sievers (Created) (JIRA)" <ji...@apache.org> on 2011/10/21 17:46:32 UTC
[jira] [Created] (SHINDIG-1647) Provide an abstraction between
implementations and the configurations they use
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
[jira] [Updated] (SHINDIG-1647) Provide an abstraction between
implementations and the configurations they use
Posted by "Stanton Sievers (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/SHINDIG-1647?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]
Stanton Sievers updated SHINDIG-1647:
-------------------------------------
Affects Version/s: (was: 2.5.0-beta1)
Fix Version/s: (was: 2.5.0-beta1)
> 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
> Reporter: Stanton Sievers
>
> 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
[jira] [Commented] (SHINDIG-1647) Provide an abstraction between
implementations and the configurations they use
Posted by "jiraposter@reviews.apache.org (Commented) (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/SHINDIG-1647?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13134209#comment-13134209 ]
jiraposter@reviews.apache.org commented on SHINDIG-1647:
--------------------------------------------------------
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/2467/
-----------------------------------------------------------
(Updated 2011-10-24 16:28:55.659592)
Review request for Ryan Baxter, Dan Dumont and Jesse Ciancetta.
Changes
-------
Updates based on feedback.
Summary
-------
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.
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 addresses bug SHINDIG-1647.
https://issues.apache.org/jira/browse/SHINDIG-1647
Diffs (updated)
-----
http://svn.apache.org/repos/asf/shindig/trunk/java/common/src/main/java/org/apache/shindig/config/ValueProvider.java PRE-CREATION
Diff: https://reviews.apache.org/r/2467/diff
Testing
-------
No specific testing done on the patch, but testing was done in the other review for those classes that implement and use it.
Thanks,
Stanton
> 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
[jira] [Commented] (SHINDIG-1647) Provide an abstraction between
implementations and the configurations they use
Posted by "jiraposter@reviews.apache.org (Commented) (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/SHINDIG-1647?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13134424#comment-13134424 ]
jiraposter@reviews.apache.org commented on SHINDIG-1647:
--------------------------------------------------------
bq. On 2011-10-24 18:14:22, Dan Dumont wrote:
bq. > 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 :)
bq. > 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.
bq. > ---
bq. >
bq. > I'll respond to a few things from below.
bq. >
bq. > > I still feel like this is really complicated and heavy-weight for doing pretty
bq. > > much what ContainerConfig already does today.
bq. >
bq. > 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.
bq. > 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. >
bq. > > but everything else appears to be essentially just re-implementing a bunch of
bq. > > the existing functionality.
bq. > 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.
bq. > 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. >
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.
bq. > 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. >
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.
bq. > 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. >
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.
bq. > 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.
bq. >
bq. > 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. >
bq. > > Sorry for being the squeaky wheel on this set of changes.
bq. > Not at all! I'm glad to have such thorough reviews. Let me know if any of this makes any sense. :)
bq. >
bq. >
bq. >
bq.
bq. Jesse Ciancetta wrote:
bq. >I'll respond to a few things from below.
bq. >
bq. >> I still feel like this is really complicated and heavy-weight for doing pretty
bq. >> much what ContainerConfig already does today.
bq. >
bq. >It is, somewhat. Especially for the example we are committing. I admit it does seem a bit over the top for something
bq. >as simple as a key provider.
bq. >Keep in mind though, the new interface is so that we can inject it and swap out implementations. Our key provider
bq. >will behave differently.
bq.
bq. Yup -- I get that. But I think we could implement a guice-injectable model on top of ContanerConfig too without much trouble too.
bq.
bq. >> but everything else appears to be essentially just re-implementing a bunch of
bq. >> the existing functionality.
bq. >One main point is that it's mostly all common code now (for the things that use this pattern). Unless you are caching
bq. >or deriving some value from the ProvidedValue, there now no need for code to ever have to observe the container.
bq. >This provides a common structure so that others won't fall into concurrency traps, and have duplicated code all over
bq. >the place (there's tons of ContainerConfig observer stuff all over shindig, some classes are independently watching the
bq. >same config settings)
bq.
bq. But it seems we're just swapping listening to ContainerConfig change events to now listening for ProvidedValue change events. We still have a KeyProvidedValueListener and a DomainProvidedValueListener inside of the BlobCrypterSecurityTokenCodec except now they're inner classes that implement ValueChangeListener instead of having the BlobCrypterSecurityTokenCodec implement ConfigObserver. But I don’t see any reason why we couldn't just as easily make the ConfigObserver implementation an inner class too to break it up too.
bq.
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.
bq. >The interface is so that we can inject a different implementation, not so much that we needed it to implement a
bq. >ValueProvider. We wanted to maintain back-compat.
bq.
bq. Again -- I think there are ways we could make guice injecting a ContainerConfig based approach just as easy, and probably do it without having to add per-class-specific extensibility interfaces.
bq.
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.
bq. >I didn't want to replace ContainerConfig. The ValueProvider stuff uses ContainerConfig.
bq. >I think the thing I may not be explaining well is that I'm not prociding a configuration,
bq. >like "load your key from this file on start up". I'm trying to provide an actual value
bq. >(tried to be explicit in the name ValueProvider instead of ConfigProvider). "This is the
bq. >key to use, use it.". That's why the strong typing. while you could use this to provide
bq. >a value straight out of the ContainerConfig, I'd argue that this impl lends itself well to
bq. >building more complex and well structures values that may consist of several
bq. >ContainerConfig settings.
bq.
bq. Right -- the strong typing was one thing I saw that really set this abstraction apart from ContainerConfig, but I'm sort of struggling to come up with use cases where we really need it. And if you do need something more structured I think the current ContainerConfig abstraction supports using full JSON objects which seems pretty flexible to me.
bq.
bq. And I agree on changing the config value for the token key from a pointer to a file on disk somewhere that the configuration-wanting-class has to go and load to an actual value that is provided to the configuration-wanting-class -- and that little prototype I threw together was doing that (I hadn’t actually implemented the support to convert file:// and res:// entries in container.js to real values, but I don’t see any reason why we couldn’t).
bq.
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.
bq. >That depends... There are places that observe the ContainerConfig to check the value of a config setting the same way
bq. >in multiple class files. This approach lets you consolidate those into 1 injectable class, 1 instance and less clutter
bq. >with listeners for the container config.
bq.
bq. Do you have a pointer to somewhere in the codebase that is doing what you're talking about here? It might help to clarify things for me.
bq.
bq. >For instance: the locked domain suffix config. This abstraction would make it much easier to implement obtaining that
bq. >value. No listeners are needed for the classes that need it, they simply inject the provider and do a get on the
bq. >provided value. The value is updated in the provider automatically, and the implementation for the provider is quite
bq. >simple.
bq.
bq. But if you want to be notified if something changes don’t you still need to provide an instance of ValueChangeListener?
bq.
bq. >> Sorry for being the squeaky wheel on this set of changes.
bq. >Not at all! I'm glad to have such thorough reviews. Let me know if any of this makes any sense. :)
bq.
bq. It does make sense and I can see what your trying to do -- I just don’t understand why we can't do all of it (or maybe 95% of it) with ContainerConfig.
bq.
bq. I had started to update that little prototype a little more to work with a more generic guice-injectable ContainerConfigContributor which could contribute multiple values (which was one of the open questions we had on the prototype) but I haven’t had time to get back to it... If you're interested in seeing what that might look like fleshed out a little more let me know and I'll see what I can do -- maybe I could spend a little more time on it tomorrow... I (unfortunately) can't really make any guarantees though. For those interested the prototype I mentioned can be found here:
bq.
bq. https://reviews.apache.org/r/2390/
bq.
bq. Like I said before though I definitely don’t want to hold you guys up. I assume all three of you guys think this new abstraction is the right way to go and so far I'm the only one disagreeing... I think you had opened this up to the dev@ list once before and we got crickets there so I'm assuming that either no one else cares or they care but don’t feel strongly enough about it one way or another to comment -- so either way I'd have no problem with you guys going ahead and moving forward on this.
bq.
bq. Or I'm happy to keep working with you guys on it and hashing it out -- either way is completely fine with me.
I agree we could accomplish much of the same thing by extending the ContainerConfig... I just feel less comfortable with that approach.
I think there's less risk of overriding smaller parts of the code than potentially messing up something as central as the ContainerConfig class.
I'm also approaching this from the viewpoint that the configuration should support the implementation, rather than drive it. So I envision that most (if not all) default values for the config settings could move into these ValueProvider implementations and have a tight coupling of config setting to what they configure. In this way, the container config can provide commented out default config entries for much of the configuration file, with notes on where to find the configuration. I'm hoping this would reduce the ContainerConfig clutter that I've seen recently when updating some keys.
This should make things a bit easier to maintain, as you won't have config values for implementations you aren't using. (Our KeyProvider won't be watching the ContainerConfig)
And directly to your point about swapping out 1 listener for another, yes we are. However, the new listener is strongly typed and is the final, derived value rather than just the config value.
I went and looked at the code and there's quite a bit fewer config observers than I thought and some of those are questionable... but...
Even for code simply doing a config.getString(container, key), this interface would aid in allowing you to define the interpretation of the config once and in a location tightly coupled with the key it's listening to. It means less "static final String key" declarations, and will be easier to find all of the parts of code that depend on a value.
I don't know... I see your point, but I'm far more comfortable with overriding the injection of a particular value provider (bound by an interface contract) than I am overriding the ContainerConfig object and introducing some bug because the code is not expecting me to interpret a config setting a certain way or as a certain type.
I don't think the JSON types are enough. This provides the means of explicitly stating that this value is to be used for these purposes... It's localized to the particular class you are injecting, and finding the consequences of the alternate implementation are easier because there's a well defined class you can look for references on.
----
That's it for the playback. Please, if anyone has any strong feelings on this topic, chime in!
- Dan
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/2467/#review2796
-----------------------------------------------------------
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
[jira] [Commented] (SHINDIG-1647) Provide an abstraction between
implementations and the configurations they use
Posted by "jiraposter@reviews.apache.org (Commented) (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/SHINDIG-1647?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13134417#comment-13134417 ]
jiraposter@reviews.apache.org commented on SHINDIG-1647:
--------------------------------------------------------
bq. On 2011-10-21 19:34:35, Jesse Ciancetta wrote:
bq. > http://svn.apache.org/repos/asf/shindig/trunk/java/common/src/main/java/org/apache/shindig/config/ValueProvider.java, line 219
bq. > <https://reviews.apache.org/r/2467/diff/3/?file=52114#file52114line219>
bq. >
bq. > I'm guessing this is supposed to be:
bq. >
bq. > cache.putIfAbsent(container, Maps.newConcurrentMap());
bq. >
bq. > ?
bq. >
bq. > Or maybe this is the way it was intended which (I think) would make the next line not needed?
bq. >
bq. > I think I'd still do it in two lines either way though.
bq.
bq. Ryan Baxter wrote:
bq. Jesse see my comment above and Dan's response.
Thanks Ryan. Dan's previous comment helps to make it more clear, but the assignment to oldValues inside of the call to putIfAbsent(...) still doesn't look quite right/needed to me. I think just removing that assignment and adding a comment inside the if block would clear things up:
ConcurrentMap<String, Object> oldValues = cache.get(container);
if (oldValues == null) {
//There wasnt a value in the cache yet when we checked a couple of lines ago, but there could
//be by now if another thread already executed the code below us... So to be safe we'll call
//putIfAbsent(...) and then go ahead and fetch the value again -- this should keep everything
//thread safe... See the docs on putIfAbsent(...) for more detail.
cache.putIfAbsent(container, Maps.<String, Object>newConcurrentMap());
oldValues = cache.get(container);
}
- Jesse
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/2467/#review2753
-----------------------------------------------------------
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
[jira] [Commented] (SHINDIG-1647) Provide an abstraction between
implementations and the configurations they use
Posted by "jiraposter@reviews.apache.org (Commented) (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/SHINDIG-1647?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13134208#comment-13134208 ]
jiraposter@reviews.apache.org commented on SHINDIG-1647:
--------------------------------------------------------
bq. On 2011-10-21 19:34:35, Jesse Ciancetta wrote:
bq. > http://svn.apache.org/repos/asf/shindig/trunk/java/common/src/main/java/org/apache/shindig/config/ValueProvider.java, line 162
bq. > <https://reviews.apache.org/r/2467/diff/3/?file=52114#file52114line162>
bq. >
bq. > Extraneous semicolon
That's what I get for doing JavaScript and Java development at the same time. :)
bq. On 2011-10-21 19:34:35, Jesse Ciancetta wrote:
bq. > http://svn.apache.org/repos/asf/shindig/trunk/java/common/src/main/java/org/apache/shindig/config/ValueProvider.java, line 255
bq. > <https://reviews.apache.org/r/2467/diff/3/?file=52114#file52114line255>
bq. >
bq. > I think initialized might need to be declared as volatile for this double-checked-locking to work properly.
bq. >
bq. > Since this method is really only called when Shindig is initializing it might be simpler to just synchronize the whole method.
This isn't necessarily called when Shindig is initializing; rather, it's called the first time anyone needs to get a value of when they add a listener. The reason to not synchronize the whole method is because I don't want threads to wait on a no-op in the case of any call after the first.
And yes, I believe you are right that this should be volatile in this case, so long as I keep the synchronization around the read/write of its value.
- Stanton
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/2467/#review2753
-----------------------------------------------------------
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
[jira] [Closed] (SHINDIG-1647) Provide an abstraction between
implementations and the configurations they use
Posted by "Paul Lindner (Closed) (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/SHINDIG-1647?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]
Paul Lindner closed SHINDIG-1647.
---------------------------------
part of 2.5.0-beta1 release.
> 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: 2.5.0-beta1
> Reporter: Stanton Sievers
> Fix For: 2.5.0-beta1
>
>
> 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
[jira] [Commented] (SHINDIG-1647) Provide an abstraction between
implementations and the configurations they use
Posted by "jiraposter@reviews.apache.org (Commented) (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/SHINDIG-1647?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13134315#comment-13134315 ]
jiraposter@reviews.apache.org 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
[jira] [Commented] (SHINDIG-1647) Provide an abstraction between
implementations and the configurations they use
Posted by "jiraposter@reviews.apache.org (Commented) (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/SHINDIG-1647?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13132763#comment-13132763 ]
jiraposter@reviews.apache.org commented on SHINDIG-1647:
--------------------------------------------------------
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/2467/
-----------------------------------------------------------
(Updated 2011-10-21 15:48:48.709102)
Review request for Ryan Baxter, Dan Dumont and Jesse Ciancetta.
Changes
-------
Adding JIRA
Summary
-------
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.
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 addresses bug SHINDIG-1647.
https://issues.apache.org/jira/browse/SHINDIG-1647
Diffs
-----
http://svn.apache.org/repos/asf/shindig/trunk/java/common/src/main/java/org/apache/shindig/config/ValueProvider.java PRE-CREATION
Diff: https://reviews.apache.org/r/2467/diff
Testing
-------
No specific testing done on the patch, but testing was done in the other review for those classes that implement and use it.
Thanks,
Stanton
> 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
[jira] [Commented] (SHINDIG-1647) Provide an abstraction between
implementations and the configurations they use
Posted by "jiraposter@reviews.apache.org (Commented) (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/SHINDIG-1647?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13133049#comment-13133049 ]
jiraposter@reviews.apache.org commented on SHINDIG-1647:
--------------------------------------------------------
bq. On 2011-10-21 19:34:35, Jesse Ciancetta wrote:
bq. > http://svn.apache.org/repos/asf/shindig/trunk/java/common/src/main/java/org/apache/shindig/config/ValueProvider.java, line 219
bq. > <https://reviews.apache.org/r/2467/diff/3/?file=52114#file52114line219>
bq. >
bq. > I'm guessing this is supposed to be:
bq. >
bq. > cache.putIfAbsent(container, Maps.newConcurrentMap());
bq. >
bq. > ?
bq. >
bq. > Or maybe this is the way it was intended which (I think) would make the next line not needed?
bq. >
bq. > I think I'd still do it in two lines either way though.
Jesse see my comment above and Dan's response.
- Ryan
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/2467/#review2753
-----------------------------------------------------------
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
[jira] [Resolved] (SHINDIG-1647) Provide an abstraction between
implementations and the configurations they use
Posted by "Stanton Sievers (Resolved) (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/SHINDIG-1647?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]
Stanton Sievers resolved SHINDIG-1647.
--------------------------------------
Resolution: Later
> 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
[jira] [Assigned] (SHINDIG-1647) Provide an abstraction between
implementations and the configurations they use
Posted by "Stanton Sievers (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/SHINDIG-1647?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]
Stanton Sievers reassigned SHINDIG-1647:
----------------------------------------
Assignee: Stanton Sievers
> 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
> Reporter: Stanton Sievers
> Assignee: Stanton Sievers
>
> 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
[jira] [Commented] (SHINDIG-1647) Provide an abstraction between
implementations and the configurations they use
Posted by "jiraposter@reviews.apache.org (Commented) (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/SHINDIG-1647?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13134445#comment-13134445 ]
jiraposter@reviews.apache.org commented on SHINDIG-1647:
--------------------------------------------------------
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/2467/
-----------------------------------------------------------
(Updated 2011-10-24 20:26:31.623226)
Review request for shindig, Ryan Baxter, Dan Dumont, and Jesse Ciancetta.
Changes
-------
Adding the dev list. Pardon the large backlog of discussion. I wanted to make sure this approach was sane before getting everyone involved.
Summary
-------
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.
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 addresses bug SHINDIG-1647.
https://issues.apache.org/jira/browse/SHINDIG-1647
Diffs
-----
http://svn.apache.org/repos/asf/shindig/trunk/java/common/src/main/java/org/apache/shindig/config/ValueProvider.java PRE-CREATION
Diff: https://reviews.apache.org/r/2467/diff
Testing
-------
No specific testing done on the patch, but testing was done in the other review for those classes that implement and use it.
Thanks,
Stanton
> 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
[jira] [Commented] (SHINDIG-1647) Provide an abstraction between
implementations and the configurations they use
Posted by "jiraposter@reviews.apache.org (Commented) (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/SHINDIG-1647?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13134412#comment-13134412 ]
jiraposter@reviews.apache.org commented on SHINDIG-1647:
--------------------------------------------------------
bq. On 2011-10-24 18:14:22, Dan Dumont wrote:
bq. > 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 :)
bq. > 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.
bq. > ---
bq. >
bq. > I'll respond to a few things from below.
bq. >
bq. > > I still feel like this is really complicated and heavy-weight for doing pretty
bq. > > much what ContainerConfig already does today.
bq. >
bq. > 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.
bq. > 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. >
bq. > > but everything else appears to be essentially just re-implementing a bunch of
bq. > > the existing functionality.
bq. > 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.
bq. > 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. >
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.
bq. > 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. >
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.
bq. > 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. >
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.
bq. > 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.
bq. >
bq. > 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. >
bq. > > Sorry for being the squeaky wheel on this set of changes.
bq. > Not at all! I'm glad to have such thorough reviews. Let me know if any of this makes any sense. :)
bq. >
bq. >
bq. >
bq. I'll respond to a few things from below.
bq.
bq. > I still feel like this is really complicated and heavy-weight for doing pretty
bq. > much what ContainerConfig already does today.
bq.
bq. It is, somewhat. Especially for the example we are committing. I admit it does seem a bit over the top for something
bq. as simple as a key provider.
bq. Keep in mind though, the new interface is so that we can inject it and swap out implementations. Our key provider
bq. will behave differently.
Yup -- I get that. But I think we could implement a guice-injectable model on top of ContanerConfig too without much trouble too.
bq. > but everything else appears to be essentially just re-implementing a bunch of
bq. > the existing functionality.
bq. One main point is that it's mostly all common code now (for the things that use this pattern). Unless you are caching
bq. or deriving some value from the ProvidedValue, there now no need for code to ever have to observe the container.
bq. This provides a common structure so that others won't fall into concurrency traps, and have duplicated code all over
bq. the place (there's tons of ContainerConfig observer stuff all over shindig, some classes are independently watching the
bq. same config settings)
But it seems we're just swapping listening to ContainerConfig change events to now listening for ProvidedValue change events. We still have a KeyProvidedValueListener and a DomainProvidedValueListener inside of the BlobCrypterSecurityTokenCodec except now they're inner classes that implement ValueChangeListener instead of having the BlobCrypterSecurityTokenCodec implement ConfigObserver. But I don’t see any reason why we couldn't just as easily make the ConfigObserver implementation an inner class too to break it up too.
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.
bq. The interface is so that we can inject a different implementation, not so much that we needed it to implement a
bq. ValueProvider. We wanted to maintain back-compat.
Again -- I think there are ways we could make guice injecting a ContainerConfig based approach just as easy, and probably do it without having to add per-class-specific extensibility interfaces.
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.
bq. I didn't want to replace ContainerConfig. The ValueProvider stuff uses ContainerConfig.
bq. I think the thing I may not be explaining well is that I'm not prociding a configuration,
bq. like "load your key from this file on start up". I'm trying to provide an actual value
bq. (tried to be explicit in the name ValueProvider instead of ConfigProvider). "This is the
bq. key to use, use it.". That's why the strong typing. while you could use this to provide
bq. a value straight out of the ContainerConfig, I'd argue that this impl lends itself well to
bq. building more complex and well structures values that may consist of several
bq. ContainerConfig settings.
Right -- the strong typing was one thing I saw that really set this abstraction apart from ContainerConfig, but I'm sort of struggling to come up with use cases where we really need it. And if you do need something more structured I think the current ContainerConfig abstraction supports using full JSON objects which seems pretty flexible to me.
And I agree on changing the config value for the token key from a pointer to a file on disk somewhere that the configuration-wanting-class has to go and load to an actual value that is provided to the configuration-wanting-class -- and that little prototype I threw together was doing that (I hadn’t actually implemented the support to convert file:// and res:// entries in container.js to real values, but I don’t see any reason why we couldn’t).
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.
bq. That depends... There are places that observe the ContainerConfig to check the value of a config setting the same way
bq. in multiple class files. This approach lets you consolidate those into 1 injectable class, 1 instance and less clutter
bq. with listeners for the container config.
Do you have a pointer to somewhere in the codebase that is doing what you're talking about here? It might help to clarify things for me.
bq. For instance: the locked domain suffix config. This abstraction would make it much easier to implement obtaining that
bq. value. No listeners are needed for the classes that need it, they simply inject the provider and do a get on the
bq. provided value. The value is updated in the provider automatically, and the implementation for the provider is quite
bq. simple.
But if you want to be notified if something changes don’t you still need to provide an instance of ValueChangeListener?
bq. > Sorry for being the squeaky wheel on this set of changes.
bq. Not at all! I'm glad to have such thorough reviews. Let me know if any of this makes any sense. :)
It does make sense and I can see what your trying to do -- I just don’t understand why we can't do all of it (or maybe 95% of it) with ContainerConfig.
I had started to update that little prototype a little more to work with a more generic guice-injectable ContainerConfigContributor which could contribute multiple values (which was one of the open questions we had on the prototype) but I haven’t had time to get back to it... If you're interested in seeing what that might look like fleshed out a little more let me know and I'll see what I can do -- maybe I could spend a little more time on it tomorrow... I (unfortunately) can't really make any guarantees though. For those interested the prototype I mentioned can be found here:
https://reviews.apache.org/r/2390/
Like I said before though I definitely don’t want to hold you guys up. I assume all three of you guys think this new abstraction is the right way to go and so far I'm the only one disagreeing... I think you had opened this up to the dev@ list once before and we got crickets there so I'm assuming that either no one else cares or they care but don’t feel strongly enough about it one way or another to comment -- so either way I'd have no problem with you guys going ahead and moving forward on this.
Or I'm happy to keep working with you guys on it and hashing it out -- either way is completely fine with me.
- Jesse
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/2467/#review2796
-----------------------------------------------------------
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
[jira] [Commented] (SHINDIG-1647) Provide an abstraction between
implementations and the configurations they use
Posted by "jiraposter@reviews.apache.org (Commented) (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/SHINDIG-1647?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13132965#comment-13132965 ]
jiraposter@reviews.apache.org 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