brooklyn-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Richard Downer <rich...@apache.org>
Subject Re: Consistency of our getConfig/setConfig methods
Date Mon, 29 Sep 2014 14:11:15 GMT
Hmm, slightly behind on my email.

So further to my earlier message, I'm now happy that this isn't going
to cause a *huge* amount of disruption for our users, and the benefit
of cleaning up the entity base class and generalising the
configuration code exceeds that cost.

Richard.


On 9 September 2014 23:51, Aled Sage <aled.sage@gmail.com> wrote:
> Hi Richard,
>
> Good points. I prefer your name of `configuration()`.
>
> Impact on users would be that authors of the more complicated entities would
> need to refactor their usage of `getAllConfig()`, `getAllConfigBag()`, etc.
> I view some of that as inevitable anyway, as the API needs a cleanup and we
> need to make entity+location+policy APIs consistent for the long-term
> benefit of our users. Better to do that sooner rather than later.
>
> Most "simple" entities (e.g. for Tomcat and JBoss app server) just call
> `getConfig(ConfigKey)` and occassionally `setConfig(ConfigKey, val)`. We
> could leave those two duplicated on Entity as conveniences, with javadoc
> that it delegates to `configuration().getConfig()` etc.
>
> As we are moving towards less Java for most new entities (and will slowly
> refactoring existing ones), I'm not too worried about the refactoring
> involved for power users.
> (But that's a different discussion thread).
>
> ---
> So the big question I think is: will this make the interface nicer to use +
> support (given we're changing some of entity/location/policy anyway)?
>
> Aled
>
>
>
> On 25/08/2014 10:54, Richard Downer wrote:
>>
>> Hi Aled,
>>
>> Having a consistent set of methods for each type - good
>> The code for managing configuration to be in the base type and shared
>> for all implementations - good
>> Putting all the configuration into a separate class - not sure
>>
>> What's the impact on our users, and what are the benefits for them?
>>
>> I can see that architecturally it's a good idea (a class should only
>> do one thing...) but I'm not yet convinced that on practical levels,
>> the benefits outweigh the costs of the change?
>>
>> And I think there is a better name than `getConfigSupport()` - maybe
>> just `configuration()...` </bikeshedding>
>>
>> Richard.
>>
>>
>> On 22 August 2014 12:57, Aled Sage <aled.sage@gmail.com> wrote:
>>>
>>> Hi all,
>>>
>>> I'd like us to make the config methods the same on Entity, Location,
>>> Policy
>>> and Enricher.
>>>
>>> As well as making things easier to understand/use, it will also allow for
>>> more code reuse (i.e. deleting code duplication) by pushing this up into
>>> the
>>> super-type BrooklynObject.
>>>
>>> ---
>>> PROPOSAL
>>> We add `BrooklynObject.getConfigSupport()` which returns `ConfigSupport`,
>>> which has a rationalised set of methods for accessing + setting config.
>>>
>>> We deprecate (most of?) the methods on Entity etc.
>>>
>>> We stop needing to use `EntityLocal.setConfig`, which currently causes a
>>> whole load of ugly casts in our code!
>>>
>>> ---
>>> JUSTIFICATION
>>>
>>>   * It's confusing (and results in more code) that the methods are
>>>     different on Entity versus Policy etc.
>>>   * We have nine methods on Entity/EntityLocal for getConfig, setConfig
>>>     or getConfigRaw.
>>>     That number easily justifies grouping them in a class with something
>>>     like `getConfigSupport`.
>>>     On Location, we have size methods (with an overlap of two on Entity!)
>>>
>>> ---
>>> QUESTION:
>>> Which top-level methods on Entity do we keep?
>>> Is `entity.getConfig(configKey)` called so often that we don't want to
>>> force
>>> `entity.getConfigSupport().getConfig(configKey)`?
>>> Any others?
>>>
>>> ---
>>> PROPOSAL PART TWO
>>> We could do the same for attributes/sensors (which are currently only
>>> available on Entities, so that is where we'd put `getAttributeSupport`).
>>> Currently one casts to be able to call `EntityLocal.setAttribute`.
>>>
>>> Alternatively, `setAttribute` could be moved to be a top-level method on
>>> `Entity`.
>>>
>>>
>

Mime
View raw message