brooklyn-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Alasdair Hodge <alasdair.ho...@cloudsoftcorp.com>
Subject Re: [PROPOSAL] Deprecate @SetFromFlag
Date Wed, 23 Nov 2016 05:16:48 GMT
Excellent write-up, Aled.

I wholly support this, as someone who fastidiously avoids the 
'setFromFlag' names having been tripped up too many times by the 
inconsistent inheritance and other "surprises" you mention.

A.
-- 
Alasdair Hodge
Principal Engineer,
Cloudsoft Corporation


On 23/11/2016 01:30, Aled Sage wrote:
> Alex,
>
> Sticking with the two proposals being discussed separately...
>
> The problems that deprecating @SetFromFlag (and moving the naming into
> config key) is solving are:
>
> 1. The behaviour is currently inconsistent and thus confusing - e.g.
>    the name from @SetFromFlag will not be inherited, but the config key
>    name will be (so "env" is not inherited but "shell.env" will be).
>    People can't easily tell how things will behave when writing
>    blueprints based on existing examples.
> 2. The name in @SetFromFlag is not part of the entity definition - it
>    cannot be discovered on the entity type, so could never be used by a
>    YAML composer. Moving aliases (deprecated or otherwise) into the
>    config key definition makes it a proper part of the type. It can
>    thus be included in docs as well.
> 3. We have no way to rename a YAML config key, defined in a
>    brooklyn.parameters section, deprecating the old name.
>    If a better name is agreed upon (e.g. correcting spelling etc) then
>    one either has to break backwards compatibility or jump through
>    hoops in bash/freemarker to respect both names.
> 4. Our code for handling @SetFromFlag is pretty horrible - mostly
>    because it is set via a different code path from config keys,
>    leading again to inconsistencies and difficult-to-maintain code.
>
> This proposal is complementary/separate from
> https://github.com/apache/brooklyn-server/pull/363 - I don't think that
> PR (or continuation of that work) would solve these problems. YOML does
> not change the nature of config keys, or EntitySpec's flag/config.
>
> Aled
>
>
> On 22/11/2016 21:44, Aled Sage wrote:
>> Hi all,
>>
>> TL;DR: deprecate `@SetFromFlag`; instead explicitly set
>> deprecatedAlias (and/or alias?) on the ConfigKey.
>>
>> We should change this *after* releasing 0.10.0, to decrease risk.
>>
>> _*Current Situation*_
>> When writing a Java entity, one can declare a config key type as a
>> static field. One can also use the annotation `@SetFromFlag` on this.
>> The annotation allows one to include an alternative name for the key.
>>
>> This alternative name is often confusing. It is not part of the formal
>> "config key" definition (i.e. not available via
>> entity.getEntityType()), and is only respected in some situations when
>> supplying config values (e.g. when supplying config directly to an
>> entity, but not when using config inheritance).
>>
>> The `@SetFromFlag` annotation also supports "defaultVal", but this is
>> (hopefully!) never used. There is a better way to specify a default
>> value, which is to pass it into the Config Key's definition.
>>
>> The other options on the `@SetFromFlag` annotation are "immutable" and
>> "nullable". These are ignored in some/many situations, so are (I
>> believe) a bad idea to use (certainly bad to rely on). It's better to
>> use `.constraint(Predicates.notNull())` on the config key, rather than
>> "nullable".
>>
>> The reason for supporting multiple names can be split into (at least?)
>> three different use-cases (we'll discuss the second and third in a
>> separate email thread):
>>
>> 1. Backwards compatibility (e.g. because we already support two names,
>>    so need to keep doing that; or because we want to rename a config
>>    key, such as correcting its spelling).
>>    This use-case could be reworded as the need to support deprecated
>> names.
>> 2. Aliases (i.e. a deliberate desire to support different names,
>>    because those different names are seen as good).
>> 3. Hints on blueprint validation in a composer (e.g.
>>    "environment.variables not valid; did you mean env?")
>>
>> _*Proposal*_
>> We should stop using `@SetFromFlag` on config keys entirely -
>> deprecating its use, and warning anywhere it's used.
>>
>> Instead, we should add support for multiple names on the ConfigKey
>> definition itself (i.e. to the ConfigKey interface). This will make it
>> part of the entity's type. This will make it easier to support in yaml
>> blueprints, and also possible to deprecate config key names in YAML
>> brooklyn.parameters.
>>
>> I suggest we support something like this:
>>
>>    ConfigKey<String> PRE_INSTALL_COMMAND =
>>    ConfigKeys.builder(String.class, "my.toy.example")
>>             .description("...")
>>             .defaultVal("myDefaultVal")
>>    .deprecatedAlias("my.toyy.example") // e.g. deprecated to correct
>>    the misspelling
>>    .deprecatedAliasSince("my.old.example", "0.10.0") // second arg is
>>    version when it was deprecated
>>             .build();
>>
>> Or in YAML:
>>
>>    brooklyn.parameters:
>>    - name: my.toy.example
>>       type: String
>>       description: ...
>>       default: myDefaultVal
>>       deprecatedAliases:
>>       - name: my.old.example
>>         deprecatedSince: 0.10.0
>>         # The version above is optional; it will also accept the name
>>    by itself:
>>       - my.toyy.example
>>
>> Here is a real-world example in Java (getting rid of
>> `@SetFromFlag("preInstallCommand")`):
>>
>>    ConfigKey<String> PRE_INSTALL_COMMAND =
>>    ConfigKeys.builder(String.class, "pre.install.command")
>>             .description("Command to be run prior to the install method
>>    being called on the driver")
>>    .runtimeInheritance(BasicConfigInheritance.NOT_REINHERITED)
>>             .deprecatedName("preInstallCommand")
>>             .build();
>>
>> When parsing the entity's Java config keys, we should be able to
>> translate the `@SetFromFlag` into an equivalent "deprecated aliases"
>> on the Config Key object (while preserving backwards compatibility).
>> When we can delete the `@SetFromFlag` support entirely, it will really
>> clean up several areas of ugly/fiddly code!
>>
>> ==========
>> Part 2
>> ==========
>>
>> We should discuss *in a different thread* whether we want to support
>> multiple aliases that are deliberately not deprecated (e.g.
>> "shell.env" and "env"). See email thread "[PROPOSAL] Deprecate config
>> key aliases" to follow shortly.
>>
>> If desired, we could support `.alias("my.toy.example.otherName")` on
>> the config key.
>>
>> ==========
>> Part 3
>> ==========
>>
>> We currently also support the `@SetFromFlag` annotation on fields
>> (let's focus here on entity, location, enricher and policy).
>>
>> The value of the field is persisted, and is set on rebind. It behaves
>> like an attribute that can be set via a config key. Its use has been
>> discouraged (on entities, at least) for quite some time, but not
>> formally deprecated.
>>
>> _Proposal_
>> For entities, we should formally deprecated this - warning in any
>> entity that uses it - and delete support for it in a couple of
>> releases time.
>>
>> For locations/policies/enrichers, I think we should also deprecated
>> it. Unfortunately they don't support "attributes", but they do support
>> reconfigurable config so that can be used instead.
>>
>>
>> ==========
>> Part 4
>> ==========
>>
>> One can use @SetFromFlag on fields of other objects, which allows the
>> fields to be set when using the DSL `$brooklyn:object`.
>>
>> I think this will become redundant with Alex's YOML. Let's not discuss
>> such usage of @SetFromFlag in this email thread.
>>
>>
>
>



Mime
View raw message