Configuration variable changes...

[Alex Huang <Al...@citrix.com>]

As part of the work to pull apart orchestration from self service, I made some changes to how configuration parameters work.  The problem with the current system are as follows:

- configuration variables are all stored as enums in Config.java which means plugins have to modify a single file.  We established that to be a bad pattern in some earlier thread.
- No way to tell during upgrades whether a config variable has become useless or if the defaults have changed.
- No way to consistently have variables be dynamically updated.
- No way to consistently migrate a global variable to a scoped variable.
- No way to use more than one type of storage (db) to store config variables.
- Some of the code are still using text strings to retrieve configuration.
- No way to consistently validate variables.  (although this is not done yet but I described how it can be done in this new framework.)

The changes are detailed on wiki [1].  There's a detail list of todo items in ConfigDepotImpl.java if you're interested in picking up any of the work.  The old way still works but I recommend we move all new way for new config parameters.

If everyone reviewed it all and like how it works then we can remove the old way of how it all works.

--Alex
[1] https://cwiki.apache.org/confluence/display/CLOUDSTACK/Configuration

Re: Configuration variable changes...

[Darren Shepherd <da...@gmail.com>]

On 09/11/2013 09:40 AM, Alex Huang wrote:

>
> I can probably add a method with the exact same signature but really all it does will be check the type and cast.
>

Yeah that's all I want.  It's just so you aren't forced to do 
(Integer)depot.get("mynumber") because that would be an unsafe cast and 
then the compiler warns and all that.

Darren

RE: Configuration variable changes...

[Alex Huang <Al...@citrix.com>]

> Can the generic API be
> 
> 	<T> ConfigKey<T> get(String paramName, Class<T> type);
> 
> Just to be a little easier to use.  I know the method is discouraged, but I do so
> some places it would be useful.  Besides that I like it.
> The contract is simple enough that the implementation can become quite
> flexible.

A similar usage is already on the ConfigDepot interface.  

ConfigKey<?> get(String key);

I can probably add a method with the exact same signature but really all it does will be check the type and cast.

> 
> Scoped values seems a little strange though.  Why do you define the scope in
> the key?  Why wouldn't you pass the scope when you call valueIn?  Its seems
> a bit odd.  Because as the caller, you need to know what the Long is (is it a
> zone id, pool id, etc.).  So it seems more natural to pass valueIn(Scope.Zone,
> 42) because it is very explicit to the caller.  Also, why can't I have a key that is
> scoped at a different level depending on the calling context.

That's a good question.  I had the same question when I looked at the original implementation of scoped variables.  The scope is defined in both the key and when getting value but they're always the same so essentially it's defined in the key and not the get value.  I agree with your assessment that it's much more flexible to have the scope defined when getting the value, but since I'm mostly refactoring here, I didn't want to get too far away from the original intent of the scope code so I left the scope definition in the key for now.  Maybe the original implementer can take a look to see if that flexibility makes sense for their requirements. 

I can see for example that at least the original implementation does not really have a true scope implementation where it checks cluster, then pod, then zone, then global.  It only checked the scope that was defined in the key.  It would have been much better if the scope in the key defined the depth of the scope and the scope in get value defined where to start search for the override within that depth.
 
--Alex

Re: Configuration variable changes...

[Darren Shepherd <da...@gmail.com>]

On 09/09/2013 10:20 AM, Alex Huang wrote:
> As part of the work to pull apart orchestration from self service, I made some changes to how configuration parameters work.  The problem with the current system are as follows:
>
> - configuration variables are all stored as enums in Config.java which means plugins have to modify a single file.  We established that to be a bad pattern in some earlier thread.
> - No way to tell during upgrades whether a config variable has become useless or if the defaults have changed.
> - No way to consistently have variables be dynamically updated.
> - No way to consistently migrate a global variable to a scoped variable.
> - No way to use more than one type of storage (db) to store config variables.
> - Some of the code are still using text strings to retrieve configuration.
> - No way to consistently validate variables.  (although this is not done yet but I described how it can be done in this new framework.)
>
> The changes are detailed on wiki [1].  There's a detail list of todo items in ConfigDepotImpl.java if you're interested in picking up any of the work.  The old way still works but I recommend we move all new way for new config parameters.
>
> If everyone reviewed it all and like how it works then we can remove the old way of how it all works.
>
> --Alex
> [1] https://cwiki.apache.org/confluence/display/CLOUDSTACK/Configuration
>

Can the generic API be

	<T> ConfigKey<T> get(String paramName, Class<T> type);

Just to be a little easier to use.  I know the method is discouraged, 
but I do so some places it would be useful.  Besides that I like it. 
The contract is simple enough that the implementation can become quite 
flexible.

Scoped values seems a little strange though.  Why do you define the 
scope in the key?  Why wouldn't you pass the scope when you call 
valueIn?  Its seems a bit odd.  Because as the caller, you need to know 
what the Long is (is it a zone id, pool id, etc.).  So it seems more 
natural to pass valueIn(Scope.Zone, 42) because it is very explicit to 
the caller.  Also, why can't I have a key that is scoped at a different 
level depending on the calling context.

Darren

Re: Configuration variable changes...

[Darren Shepherd <da...@gmail.com>]

On 09/09/2013 12:41 PM, Alex Huang wrote:
 > The problem is the second constructor saved me a lot of typing when I 
convert from the enums in Config.java to using this class.

You could immediately deprecate the new method :)

RE: Configuration variable changes...

[Alex Huang <Al...@citrix.com>]

Daan,

Yeah... I would have preferred to use the first constructor only just because it makes more sense.  The problem is the second constructor saved me a lot of typing when I convert from the enums in Config.java to using this class.  So I kept both in there.  I think as long as it's no ambiguous, it should be okay.  If we want to make sure, we probably should remove the first constructor.  The number of things I have to type for the first one was just prohibitive.  :P

As for the multiplier, I figure its usage is probably too small to deserve another constructor.  If there's a need for it, please go ahead and add another constructor.  

--Alex

> -----Original Message-----
> From: Daan Hoogland [mailto:daan.hoogland@gmail.com]
> Sent: Monday, September 9, 2013 12:01 PM
> To: dev
> Subject: Re: Configuration variable changes...
> 
> Alex,
> 
> I looked up the constructors and figured it out. next question is about two of
> them:
> 
>     public ConfigKey(Class<T> type, String name, String category, String
> defaultValue, String description, boolean isDynamic); and
>     public ConfigKey(String category, Class<T> type, String name, String
> defaultValue, String description, boolean isDynamic);
> 
> should one of them be removed?
> 
> makes more sense to add a
>     public ConfigKey(Class<T> type, String name, String category, String
> defaultValue, String description, boolean isDynamic, T multiplier); that uses
> global scope.
> 
> 
> 
> On Mon, Sep 9, 2013 at 7:48 PM, Daan Hoogland <da...@gmail.com>
> wrote:
> > Looks great Alex,
> >
> > One question; Adding a scope or a multiplier is featured on the wiki
> > but not specified. Can you add a pointer to it?
> >
> > Very nice indeed,
> > Daan
> >
> > On Mon, Sep 9, 2013 at 7:20 PM, Alex Huang <Al...@citrix.com>
> wrote:
> >> As part of the work to pull apart orchestration from self service, I made
> some changes to how configuration parameters work.  The problem with the
> current system are as follows:
> >>
> >> - configuration variables are all stored as enums in Config.java which
> means plugins have to modify a single file.  We established that to be a bad
> pattern in some earlier thread.
> >> - No way to tell during upgrades whether a config variable has become
> useless or if the defaults have changed.
> >> - No way to consistently have variables be dynamically updated.
> >> - No way to consistently migrate a global variable to a scoped variable.
> >> - No way to use more than one type of storage (db) to store config
> variables.
> >> - Some of the code are still using text strings to retrieve configuration.
> >> - No way to consistently validate variables.  (although this is not
> >> done yet but I described how it can be done in this new framework.)
> >>
> >> The changes are detailed on wiki [1].  There's a detail list of todo items in
> ConfigDepotImpl.java if you're interested in picking up any of the work.  The
> old way still works but I recommend we move all new way for new config
> parameters.
> >>
> >> If everyone reviewed it all and like how it works then we can remove the
> old way of how it all works.
> >>
> >> --Alex
> >> [1]
> >> https://cwiki.apache.org/confluence/display/CLOUDSTACK/Configuration

Re: Configuration variable changes...

[Daan Hoogland <da...@gmail.com>]

Alex,

I looked up the constructors and figured it out. next question is
about two of them:

    public ConfigKey(Class<T> type, String name, String category,
String defaultValue, String description, boolean isDynamic);
and
    public ConfigKey(String category, Class<T> type, String name,
String defaultValue, String description, boolean isDynamic);

should one of them be removed?

makes more sense to add a
    public ConfigKey(Class<T> type, String name, String category,
String defaultValue, String description, boolean isDynamic, T
multiplier);
that uses global scope.



On Mon, Sep 9, 2013 at 7:48 PM, Daan Hoogland <da...@gmail.com> wrote:
> Looks great Alex,
>
> One question; Adding a scope or a multiplier is featured on the wiki
> but not specified. Can you add a pointer to it?
>
> Very nice indeed,
> Daan
>
> On Mon, Sep 9, 2013 at 7:20 PM, Alex Huang <Al...@citrix.com> wrote:
>> As part of the work to pull apart orchestration from self service, I made some changes to how configuration parameters work.  The problem with the current system are as follows:
>>
>> - configuration variables are all stored as enums in Config.java which means plugins have to modify a single file.  We established that to be a bad pattern in some earlier thread.
>> - No way to tell during upgrades whether a config variable has become useless or if the defaults have changed.
>> - No way to consistently have variables be dynamically updated.
>> - No way to consistently migrate a global variable to a scoped variable.
>> - No way to use more than one type of storage (db) to store config variables.
>> - Some of the code are still using text strings to retrieve configuration.
>> - No way to consistently validate variables.  (although this is not done yet but I described how it can be done in this new framework.)
>>
>> The changes are detailed on wiki [1].  There's a detail list of todo items in ConfigDepotImpl.java if you're interested in picking up any of the work.  The old way still works but I recommend we move all new way for new config parameters.
>>
>> If everyone reviewed it all and like how it works then we can remove the old way of how it all works.
>>
>> --Alex
>> [1] https://cwiki.apache.org/confluence/display/CLOUDSTACK/Configuration

Re: Configuration variable changes...

[Daan Hoogland <da...@gmail.com>]

Looks great Alex,

One question; Adding a scope or a multiplier is featured on the wiki
but not specified. Can you add a pointer to it?

Very nice indeed,
Daan

On Mon, Sep 9, 2013 at 7:20 PM, Alex Huang <Al...@citrix.com> wrote:
> As part of the work to pull apart orchestration from self service, I made some changes to how configuration parameters work.  The problem with the current system are as follows:
>
> - configuration variables are all stored as enums in Config.java which means plugins have to modify a single file.  We established that to be a bad pattern in some earlier thread.
> - No way to tell during upgrades whether a config variable has become useless or if the defaults have changed.
> - No way to consistently have variables be dynamically updated.
> - No way to consistently migrate a global variable to a scoped variable.
> - No way to use more than one type of storage (db) to store config variables.
> - Some of the code are still using text strings to retrieve configuration.
> - No way to consistently validate variables.  (although this is not done yet but I described how it can be done in this new framework.)
>
> The changes are detailed on wiki [1].  There's a detail list of todo items in ConfigDepotImpl.java if you're interested in picking up any of the work.  The old way still works but I recommend we move all new way for new config parameters.
>
> If everyone reviewed it all and like how it works then we can remove the old way of how it all works.
>
> --Alex
> [1] https://cwiki.apache.org/confluence/display/CLOUDSTACK/Configuration