brooklyn-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Aled Sage <aled.s...@gmail.com>
Subject Re: [PROPOSAL] Deprecate @SetFromFlag
Date Tue, 22 Nov 2016 23:30:03 GMT
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
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message