brooklyn-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Alex Heneveld <alex.henev...@cloudsoftcorp.com>
Subject Re: [PROPOSAL] Deprecate @SetFromFlag
Date Wed, 07 Dec 2016 13:54:03 GMT
Hi All-

I'm with Svet:  as much as we might like to deprecate things, I think we
mustn't without a clear path that allows users to move away from anything
we've deprecated.

In the case of blueprint *authors* (who might have used @SetFromFlag) we
need to consider blueprint *consumers* who may be using config aliases
defined by authors.  If we deprecate aliases -- either marking SetFromFlag
as @Deprecated or introducing a deprecatedAlias method -- we need to
provide at the same time a mechanism for authors and their blueprint
consumers to move away from those aliases.  This should be something more
user friendly than comments in the docs.

Until we have that I think it would be poor form to deprecate aliases, as
we're essentially warning that we might remove something in a subsequent
version, but the blueprint authors don't have a good way to ensure that the
blueprint consumers can migrate to the alias-free world.

I'd make a counter proposal that:

(1) ASAP we document that `@SetFromFlag` and config aliases in general are
*discouraged* and we are moving towards a migration strategy for blueprint
consumers that will allow them to be deprecated in most or all cases

Re the migration strategy `deprecatedAlias` would work at least to record
intent but I think YOML will provide a better path especially how we could
communicate that intent to blueprint consumers.  I'd rather invest energy
in that.  I'd propose we continue as follows:

(2) We work to activate YOML as the preferred mechanism for all conversion
of YAML to java objects (specs and catalog format)

(3) We add a @DeprecatedAlias tag to YOML which acts like @Alias but can
inform users it is deprecated

(4) We adjust the UI to warn on use of deprecated aliases (maybe a new
warnings view in the first instance, and we warn whenever a deprecated item
is encountered -- possibly just intercepting logback statements and making
them available)

(5) We deprecate @SetFromFlag pointing at the annotations in 3 (note that
these are still annotations because any such info is purely for the
de/serialization from YAML; it is not needed for working with the config
key once YAML has been parsed because YOML converts it to the official key
name, but see 3' below)

I'd also like to:

(6) We provide a REST endpoint that can convert to canonical form (YOML
supports this out of the box, although not currently with comments), making
the canoncial form use the official key name for consistency, and we expose
this conversion functionality in the UI

The one thing this doesn't address is changing the runtime name of a key.
Say I've got a config key whose name is `shell.env` for which `env` is an
alias, and I want to change it so that `env` is the one true name.  The
process above does not provide a way for *accessors* of the key at runtime
to migrate to the new name.  Either the key's name is "env" or "shell.env"
-- it can't be both -- so all consumption/access would have to be suddenly
switched over from `$brooklyn:config("shell.env")` to
`$brooklyn:config("env")` with no migration period.  De/serialization
aliases don't help us here.  For that I'd add:

(3') Add `deprecatedNames` to config keys, and ensure that lookups look
also for these deprecated *names* (not aliases as aliases have never been
supported for lookup)

For sensors and effectors this is not needed as there are easy ways to
propagate a sensor to a deprecated name and write an effector which
forwards to another.  But:

(4') We should also provide a way to deprecate sensors and effectors and
give warnings when deprecated items are used (and these warnings should be
surfaced in the UI)

So in short I agree with the sentiment to move away from aliases but I'm
opposed to a tactical code-level-only deprecation.  A bigger improvement is
needed and I really think the idea of a canonical form and YOML is the way
to do this.  It's not a small effort but I don't think it's as big as what
people might fear; YOML gives us a lot of power.  It will let us retire the
many different piecemeal strategies we have for working with YAML, and will
also solve type access issues (who is allowed to access which classes) and
informing consumers where things are deprecated ... which is going to be
hard however we do it.  YOML gives us a single way which can be used for
code completion, documentation, even graphical support for editing.

Best
Alex



On 7 December 2016 at 12:31, Svetoslav Neykov <
svetoslav.neykov@cloudsoftcorp.com> wrote:

> @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
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message