brooklyn-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Svetoslav Neykov <svetoslav.ney...@cloudsoftcorp.com>
Subject Re: [PROPOSAL] Deprecate @SetFromFlag
Date Wed, 07 Dec 2016 12:31:42 GMT
@SetFromFlag can be deprecated only when:
  * We have an alternative way for marking (deprecated) aliases as part of the config key
  * A plan forward what to do with the annotation when used in beans (part 4 from proposal)

It won't be too useful If we deprecate the annotation, but don't let users know how to remove
it safely from their code in a way that's compatible with existing blueprints.

Suggest leaving it for next release.

Svet.


> On 7.12.2016 г., at 14:04, Geoff Macartney <geoff.macartney@cloudsoftcorp.com>
wrote:
> 
> +1 to the proposal, I like the simplification and consistency that it will
> bring.
> 
> Is it worth getting the @Deprecated into 0.10.0?  Would we not want to also
> update the docs at the time to remove recommendations to use @SetFromFlag?
>  It would seem odd to deprecate the annotation in the Java but leave the
> docs untouched.
> 
> Geoff
> 
> 
> 
> On Wed, 7 Dec 2016 at 11:32 Richard Downer <richard@apache.org> wrote:
> 
>> +1
>> 
>> If there's another 0.10.0 release candidate, do we want to try and get that
>> "deprecated" tag in to 0.10.0?
>> 
>> Richard.
>> 
>> 
>> On 2 December 2016 at 21:16, Sam Corbett <sam.corbett@cloudsoftcorp.com>
>> wrote:
>> 
>>> 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