brooklyn-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Aled Sage <aled.s...@gmail.com>
Subject Re: Consistency of our getConfig/setConfig methods
Date Tue, 09 Sep 2014 22:51:57 GMT
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
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message