brooklyn-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Thomas Bouron <thomas.bou...@cloudsoftcorp.com>
Subject Re: [PROPOSAL] Deprecate @SetFromFlag
Date Wed, 23 Nov 2016 08:32:08 GMT
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
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message