[
https://issues.apache.org/jira/browse/SHINDIG-1647?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13134424#comment-13134424
]
[email protected] 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
