brooklyn-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Sam Corbett <sam.corb...@cloudsoftcorp.com>
Subject Re: [PROPOSAL] Deprecate @SetFromFlag
Date Fri, 02 Dec 2016 21:16:41 GMT
I don't see much to disagree with here. The SetFromFlag annotation is an 
abstraction that might have made sense briefly but is less relevant as 
we push pure-YAML blueprints. Deprecating the feature will be a 
relatively simple, unintrusive change that will make the codebase easier 
to reason with.

Sam

On 23/11/2016 11:44, Aled Sage wrote:
> Responding to some more of Alex's comments that he made in the renamed 
> email thread "[PROPOSAL] Deprecate config key aliases / SetFromFlag"...
>
> ---
> To be clear, if the conclusion from the "Deprecate config key aliases" 
> discussion is that we support multiple non-deprecated names, then this 
> proposal will support that (see "Part 2" of the original email).
>
> I'm assuming we'd *not* want aliases to deliberately have different 
> semantics (e.g. not inherited). But if we did, we can support that by 
> being explicit in the ConfigKey, rather than using the `@SetFromFlag` 
> annotation.
>
> ---
> Alex wrote:
>   "the real issue in my view is that we have too many aliases and they 
> are used inconsistently without good docs/help."
>
> This proposal gives us a clear way to improve that. The multiple names 
> would all be defined on the ConfigKey, and we'd clearly indicate which 
> are the deprecated names.
>
> Once we have that, one can incrementally clean up ConfigKey names on 
> existing entities, as desired.
>
> ---
> Alex wrote:
> "a canonical form and interactive help on keys will go a long way 
> towards solving that"
>
> Again, this proposal begins to tackle that. It makes all the name(s) 
> available on the entity's type, so they can be included in 
> auto-generated docs and so that a more sophisticated YAML composer can 
> get access to them.
>
> This proposed change is also a big enabler for the usability 
> improvements.
>
> ---
> Alex wrote:
> "simply deprecating aliases without [a canonical form and interactive 
> help on keys] is just going to be irritating."
>
> I disagree. It will make it explicit which name is the recommended 
> one, and which names are deprecated-but-still-supported.
>
> ---
> > "similarly i'd like us to have a better tie in with source control 
> and developer workflow before advocating a big change to blueprints"
>
> I don't think that deprecating @SetFromFlag is a big change to 
> blueprints. It is a big clean-up, and makes things clearer.
>
> Also, if it allows us to clean up pieces of horrible code that treat 
> `@SetFromFlag` and config key names differently, then it will make 
> subsequent big changes (e.g. to YAML parsing and entity construction) 
> easier and less risky.
>
> Aled
>
>
> On 23/11/2016 08:32, Thomas Bouron wrote:
>> Hi all.
>>
>> +1 as well from me. As a user, it is indeed very confusing to have
>> different names for the name config across our documentation.
>>
>> I also went down the inheritance hell because sometimes it didn't 
>> work for
>> the very reasons you gave above Aled. As a result, I now always write my
>> config keys within a `brooklyn.config` block, using the real config key
>> name because I am sure how it behaves at runtime.
>>
>> On Wed, 23 Nov 2016, 05:16 Alasdair Hodge, 
>> <alasdair.hodge@cloudsoftcorp.com>
>> wrote:
>>
>>> 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.
>>>>>
>>>>>
>>>>
>>>
>>> -- 
>> Thomas Bouron • Software Engineer @ Cloudsoft Corporation •
>> http://www.cloudsoftcorp.com/
>> Github: https://github.com/tbouron
>> Twitter: https://twitter.com/eltibouron
>>
>


Mime
View raw message