You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@brooklyn.apache.org by aledsage <gi...@git.apache.org> on 2014/11/11 23:54:35 UTC
[GitHub] incubator-brooklyn pull request: Refactor config methods
GitHub user aledsage opened a pull request:
https://github.com/apache/incubator-brooklyn/pull/320
Refactor config methods
Creates a BrooklynObject.config(), and deprecates the crazy number of inconsistent config accessor/setter methods we have on Entity/Location/Policy/Enricher.
Note that `AbstractEntity`, `AbstractLocation` and `AbstractEntityAdjuct` all still have their own implementation of config. Until we delete the additional deprecated access/setter methods, it will be difficult to rationalise this down into a single implementation within `AbstractBrooklynObject`. I suggest we do that as soon as we begin work on 0.8.0-SNAPSHOT.
Please merge https://github.com/apache/incubator-brooklyn/pull/319 first (for the groovy version upgrade to 2.3.4). Without that, it fails to compile with `BrooklynObject.config()` returning `ConfigurationSupport` and `BrooklynObjectInternal.config()` returning the sub-class `ConfigurationSupportInternal`.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/aledsage/incubator-brooklyn refactor/config-methods-separation-strike2
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/incubator-brooklyn/pull/320.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #320
----
commit 00e8f1179dfcf77b331fce5aa874ce53c6afbedd
Author: Aled Sage <al...@gmail.com>
Date: 2014-11-10T19:05:53Z
Add BrooklynObject.config()
- With separate implementations for Entity, Location and
AbstractEntityAdjunct.
- CatalogItemDo / CatalogItemDtoAbstract throw UnsupportedOperationException
commit 636edfcddf4913bd3afd536a14d8e5fbef686011
Author: Aled Sage <al...@gmail.com>
Date: 2014-11-11T14:48:58Z
Bump groovy version to 2.3.4 from 1.8.6
- And groovy-eclipse-compiler to 2.9.0-01 from 2.7.0-01
- And groovy-eclipse-batch to 2.3.4-01 from 1.8.6-01
commit 3ee3f8be7a66b1ec16d3d3a3054e942115f6cebf
Author: Aled Sage <al...@gmail.com>
Date: 2014-11-11T15:25:09Z
Use entity.config(), instead of newly deprecated methods
- Have deprecated much of entity.*Config methods
(all except entity.getConfig() as that is used so often).
- Changed all uses to the new config().* variants.
commit 3bb649b04e8a7e4e69fff537ad78e88bfb4a8b97
Author: Aled Sage <al...@gmail.com>
Date: 2014-11-11T16:50:17Z
Deprecate *Config methods in Location/Enricher/Policy
commit e2c3d7325f6bd94d53e93644d6866676f37544ad
Author: Aled Sage <al...@gmail.com>
Date: 2014-11-11T16:51:16Z
Use {location,policy,enricher}.config(), instead of newly deprecated methods
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-brooklyn pull request: Refactor config methods
Posted by aledsage <gi...@git.apache.org>.
Github user aledsage commented on the pull request:
https://github.com/apache/incubator-brooklyn/pull/320#issuecomment-62743178
build failed with ``Internal error: hudson.remoting.RequestAbortedException: hudson.remoting.RequestAbortedException`. Closing and re-opening to kick it off again.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-brooklyn pull request: Refactor config methods
Posted by aledsage <gi...@git.apache.org>.
Github user aledsage closed the pull request at:
https://github.com/apache/incubator-brooklyn/pull/320
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-brooklyn pull request: Refactor config methods
Posted by aledsage <gi...@git.apache.org>.
Github user aledsage commented on the pull request:
https://github.com/apache/incubator-brooklyn/pull/320#issuecomment-70005717
@grkvlt I've incorporated the review comments - could you look again please?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-brooklyn pull request: Refactor config methods
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/incubator-brooklyn/pull/320
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-brooklyn pull request: Refactor config methods
Posted by grkvlt <gi...@git.apache.org>.
Github user grkvlt commented on a diff in the pull request:
https://github.com/apache/incubator-brooklyn/pull/320#discussion_r20434339
--- Diff: api/src/main/java/brooklyn/basic/BrooklynObject.java ---
@@ -80,4 +86,51 @@
boolean removeTag(@Nonnull Object tag);
}
+ @Beta
+ public interface ConfigurationSupport {
+
+ /**
+ * Gets the given configuration value for this entity, in the following order of preference:
+ * <ol>
+ * <li> value (including null) explicitly set on the entity
+ * <li> value (including null) explicitly set on an ancestor (inherited)
+ * <li> a default value (including null) on the best equivalent static key of the same name declared on the entity
+ * (where best equivalence is defined as preferring a config key which extends another,
+ * as computed in EntityDynamicType.getConfigKeys)
+ * <li> a default value (including null) on the key itself
+ * <li> null
+ * </ol>
+ */
+ <T> T get(ConfigKey<T> key);
+
+ /**
+ * @see {@link #getConfig(ConfigKey)}
+ */
+ <T> T get(HasConfigKey<T> key);
+
+ /**
+ * Sets the config to the given value.
+ */
+ <T> T set(ConfigKey<T> key, T val);
--- End diff --
Should we support setting an `Object` value instead and doing type coercion if possible?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-brooklyn pull request: Refactor config methods
Posted by sjcorbett <gi...@git.apache.org>.
Github user sjcorbett commented on the pull request:
https://github.com/apache/incubator-brooklyn/pull/320#issuecomment-74680687
I've been through f05a6db9540c7a6a339838f0e31acf3d1ee72810 and dce1384731def10e42328849e2f1b795bad0042a and have no further comments to add to @grkvlt's from before but for two minor documentation points.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-brooklyn pull request: Refactor config methods
Posted by aledsage <gi...@git.apache.org>.
Github user aledsage commented on the pull request:
https://github.com/apache/incubator-brooklyn/pull/320#issuecomment-62709941
@grkvlt what is `BasicConfigurableObject` for? It is not used anywhere in the brooklyn code base.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-brooklyn pull request: Refactor config methods
Posted by aledsage <gi...@git.apache.org>.
Github user aledsage closed the pull request at:
https://github.com/apache/incubator-brooklyn/pull/320
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-brooklyn pull request: Refactor config methods
Posted by aledsage <gi...@git.apache.org>.
Github user aledsage commented on the pull request:
https://github.com/apache/incubator-brooklyn/pull/320#issuecomment-62710018
Build failed due to spurious "Internal error: java.lang.IllegalStateException: Invalid object ID 28 iota=55".
Closing and re-opening PR to kick off jenkins again.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-brooklyn pull request: Refactor config methods
Posted by aledsage <gi...@git.apache.org>.
Github user aledsage commented on a diff in the pull request:
https://github.com/apache/incubator-brooklyn/pull/320#discussion_r20188607
--- Diff: core/src/main/java/brooklyn/entity/rebind/BasicLocationRebindSupport.java ---
@@ -68,9 +69,9 @@ protected void addConfig(RebindContext rebindContext, LocationMemento memento) {
// FIXME Treat config like we do for entities; this code will disappear when locations become entities.
// Note that the flags have been set in the constructor
- // FIXME Relies on location.getLocalConfigBag being mutable (to modify the location's own config)
+ // FIXME Relies on location.config().getLocalBag() being mutable (to modify the location's own config)
--- End diff --
Note that I haven't dealt with this FIXME. It still works because `location.config().getLocalBag()` is returning a mutable object (despite `BrooklynObjectInternal.ConfigurationSupportInternal.getLocalBag()` javadoc saying it is read-only). I suggest we tackle that in a separate PR.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-brooklyn pull request: Refactor config methods
Posted by grkvlt <gi...@git.apache.org>.
Github user grkvlt commented on the pull request:
https://github.com/apache/incubator-brooklyn/pull/320#issuecomment-62644996
Doesn't seem to modify `BasicConfigurableObject`, is there a reason for that?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-brooklyn pull request: Refactor config methods
Posted by grkvlt <gi...@git.apache.org>.
Github user grkvlt commented on the pull request:
https://github.com/apache/incubator-brooklyn/pull/320#issuecomment-63303843
I think you have the right idea with the `config()` method, my only comment is the one about possibly allowing type coercions on input.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-brooklyn pull request: Refactor config methods
Posted by aledsage <gi...@git.apache.org>.
GitHub user aledsage reopened a pull request:
https://github.com/apache/incubator-brooklyn/pull/320
Refactor config methods
Creates a BrooklynObject.config(), and deprecates the crazy number of inconsistent config accessor/setter methods we have on Entity/Location/Policy/Enricher.
Note that `AbstractEntity`, `AbstractLocation` and `AbstractEntityAdjuct` all still have their own implementation of config. Until we delete the additional deprecated access/setter methods, it will be difficult to rationalise this down into a single implementation within `AbstractBrooklynObject`. I suggest we do that as soon as we begin work on 0.8.0-SNAPSHOT.
Please merge https://github.com/apache/incubator-brooklyn/pull/319 first (for the groovy version upgrade to 2.3.4). Without that, it fails to compile with `BrooklynObject.config()` returning `ConfigurationSupport` and `BrooklynObjectInternal.config()` returning the sub-class `ConfigurationSupportInternal`.
When reviewing, please focus on the two commits `Add BrooklynObject.config()` and `Deprecate *Config methods in Location/Enricher/Policy`. The other commits contain a massive number of boring changes to make things use the new `config().set(key, val)` rather than the old `setConfig(key, val)` etc.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/aledsage/incubator-brooklyn refactor/config-methods-separation-strike2
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/incubator-brooklyn/pull/320.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #320
----
commit 00e8f1179dfcf77b331fce5aa874ce53c6afbedd
Author: Aled Sage <al...@gmail.com>
Date: 2014-11-10T19:05:53Z
Add BrooklynObject.config()
- With separate implementations for Entity, Location and
AbstractEntityAdjunct.
- CatalogItemDo / CatalogItemDtoAbstract throw UnsupportedOperationException
commit 636edfcddf4913bd3afd536a14d8e5fbef686011
Author: Aled Sage <al...@gmail.com>
Date: 2014-11-11T14:48:58Z
Bump groovy version to 2.3.4 from 1.8.6
- And groovy-eclipse-compiler to 2.9.0-01 from 2.7.0-01
- And groovy-eclipse-batch to 2.3.4-01 from 1.8.6-01
commit 3ee3f8be7a66b1ec16d3d3a3054e942115f6cebf
Author: Aled Sage <al...@gmail.com>
Date: 2014-11-11T15:25:09Z
Use entity.config(), instead of newly deprecated methods
- Have deprecated much of entity.*Config methods
(all except entity.getConfig() as that is used so often).
- Changed all uses to the new config().* variants.
commit 3bb649b04e8a7e4e69fff537ad78e88bfb4a8b97
Author: Aled Sage <al...@gmail.com>
Date: 2014-11-11T16:50:17Z
Deprecate *Config methods in Location/Enricher/Policy
commit e2c3d7325f6bd94d53e93644d6866676f37544ad
Author: Aled Sage <al...@gmail.com>
Date: 2014-11-11T16:51:16Z
Use {location,policy,enricher}.config(), instead of newly deprecated methods
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-brooklyn pull request: Refactor config methods
Posted by aledsage <gi...@git.apache.org>.
GitHub user aledsage reopened a pull request:
https://github.com/apache/incubator-brooklyn/pull/320
Refactor config methods
Creates a BrooklynObject.config(), and deprecates the crazy number of inconsistent config accessor/setter methods we have on Entity/Location/Policy/Enricher.
Note that `AbstractEntity`, `AbstractLocation` and `AbstractEntityAdjuct` all still have their own implementation of config. Until we delete the additional deprecated access/setter methods, it will be difficult to rationalise this down into a single implementation within `AbstractBrooklynObject`. I suggest we do that as soon as we begin work on 0.8.0-SNAPSHOT.
Please merge https://github.com/apache/incubator-brooklyn/pull/319 first (for the groovy version upgrade to 2.3.4). Without that, it fails to compile with `BrooklynObject.config()` returning `ConfigurationSupport` and `BrooklynObjectInternal.config()` returning the sub-class `ConfigurationSupportInternal`.
When reviewing, please focus on the two commits `Add BrooklynObject.config()` and `Deprecate *Config methods in Location/Enricher/Policy`. The other commits contain a massive number of boring changes to make things use the new `config().set(key, val)` rather than the old `setConfig(key, val)` etc.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/aledsage/incubator-brooklyn refactor/config-methods-separation-strike2
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/incubator-brooklyn/pull/320.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #320
----
commit 00e8f1179dfcf77b331fce5aa874ce53c6afbedd
Author: Aled Sage <al...@gmail.com>
Date: 2014-11-10T19:05:53Z
Add BrooklynObject.config()
- With separate implementations for Entity, Location and
AbstractEntityAdjunct.
- CatalogItemDo / CatalogItemDtoAbstract throw UnsupportedOperationException
commit 636edfcddf4913bd3afd536a14d8e5fbef686011
Author: Aled Sage <al...@gmail.com>
Date: 2014-11-11T14:48:58Z
Bump groovy version to 2.3.4 from 1.8.6
- And groovy-eclipse-compiler to 2.9.0-01 from 2.7.0-01
- And groovy-eclipse-batch to 2.3.4-01 from 1.8.6-01
commit 3ee3f8be7a66b1ec16d3d3a3054e942115f6cebf
Author: Aled Sage <al...@gmail.com>
Date: 2014-11-11T15:25:09Z
Use entity.config(), instead of newly deprecated methods
- Have deprecated much of entity.*Config methods
(all except entity.getConfig() as that is used so often).
- Changed all uses to the new config().* variants.
commit 3bb649b04e8a7e4e69fff537ad78e88bfb4a8b97
Author: Aled Sage <al...@gmail.com>
Date: 2014-11-11T16:50:17Z
Deprecate *Config methods in Location/Enricher/Policy
commit e2c3d7325f6bd94d53e93644d6866676f37544ad
Author: Aled Sage <al...@gmail.com>
Date: 2014-11-11T16:51:16Z
Use {location,policy,enricher}.config(), instead of newly deprecated methods
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-brooklyn pull request: Refactor config methods
Posted by aledsage <gi...@git.apache.org>.
Github user aledsage commented on the pull request:
https://github.com/apache/incubator-brooklyn/pull/320#issuecomment-74256496
@grkvlt @sjcorbett @ahgittin - I've finally rebased this, so is good for review and hopefully merge. Could you take a look please?
p.s. by changing the `Configurable` interface, I suspect it will impact the Clocker downstream project that makes use of it. @grkvlt will you handle that once this is merged please?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-brooklyn pull request: Refactor config methods
Posted by grkvlt <gi...@git.apache.org>.
Github user grkvlt commented on the pull request:
https://github.com/apache/incubator-brooklyn/pull/320#issuecomment-62715577
It's a configurable object that can be extended by other projects, and is suitable for use as a `$brooklyn:object` target in a blueprint. If there's a better base class then it could be removed. I use it in Clocker as the basis for placement strategies, for example.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-brooklyn pull request: Refactor config methods
Posted by aledsage <gi...@git.apache.org>.
Github user aledsage commented on the pull request:
https://github.com/apache/incubator-brooklyn/pull/320#issuecomment-74850106
I've incorporated comments from @sjcorbett and then rebased against master.
Only comment not addressed is from @grkvlt about `<T> T set(ConfigKey<T> key, T val)`: "Should we support setting an Object value instead and doing type coercion if possible?". Let's look at that in a separate PR if we choose to do it. I'm not sure yet either way.
Will merge now.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-brooklyn pull request: Refactor config methods
Posted by sjcorbett <gi...@git.apache.org>.
Github user sjcorbett commented on the pull request:
https://github.com/apache/incubator-brooklyn/pull/320#issuecomment-71099886
@aledsage Could you resolve the merge conflicts?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-brooklyn pull request: Refactor config methods
Posted by aledsage <gi...@git.apache.org>.
Github user aledsage commented on the pull request:
https://github.com/apache/incubator-brooklyn/pull/320#issuecomment-62743355
@grkvlt before I try to change `BasicConfigurableObject` as well, any feedback on `config()` and the methods it presents?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---