You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@brooklyn.apache.org by rdowner <gi...@git.apache.org> on 2015/11/10 17:05:45 UTC
[GitHub] incubator-brooklyn pull request: Add ManagementContext.getConfigWi...
GitHub user rdowner opened a pull request:
https://github.com/apache/incubator-brooklyn/pull/1018
Add ManagementContext.getConfigWithExternalConfigResolved()
I'm not really sure about this. Throwing it out there to see what people think.
The external configuration supplier feature is great, but it doesn't work with site-wide configuration defined in brooklyn.properties. This PR is an attempt to add this function.
It, unfortunately, is not a simple matter. Because external configuration is *defined* in brooklyn.properties, there's a possible recursion problem. We could resolve this during initialisation of the management context, but we want to keep the behaviour that the configuration suppliers are invoked whenever required and always return fresh values, rather than resolving them at startup and leaving them to go stale. Some of my attempted solutions exposed weird bugs.
So the simplest possible thing is to not attempt to retro-fit this but instead add a new method which explicitly retrieves the brooklyn.properties values, with configuration resolved there and then.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/rdowner/incubator-brooklyn brooklyn-properties-external-config
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/incubator-brooklyn/pull/1018.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 #1018
----
commit e5fe7a3acce158fbd9b0cf97e69b5c7a1f779a04
Author: Richard Downer <ri...@apache.org>
Date: 2015-11-10T15:51:15Z
Add ManagementContext.getConfigWithExternalConfigResolved()
----
---
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: Add ManagementContext.getConfigWi...
Posted by rdowner <gi...@git.apache.org>.
Github user rdowner commented on the pull request:
https://github.com/apache/incubator-brooklyn/pull/1018#issuecomment-155750453
Bingo. https://github.com/apache/incubator-brooklyn/blob/master/usage/camp/src/main/java/org/apache/brooklyn/camp/brooklyn/BrooklynCampPlatform.java#L73:
```java
((BrooklynProperties)bmc.getConfig()).put(BrooklynCampConstants.CAMP_PLATFORM, this);
```
Downcasts to `BrooklynProperties` and changes the config. And if it's changing it on a copy of the config, then that change is ignored, and we end up with no CAMP platform set.
---
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: Add ManagementContext.getConfigWi...
Posted by neykov <gi...@git.apache.org>.
Github user neykov commented on the pull request:
https://github.com/apache/incubator-brooklyn/pull/1018#issuecomment-155694726
The changes look good. I like the approach because there's no hidden magic, easy to reason about. On the other hand what I don't like is that it would be very hard to figure out which config keys support external config, leaving users confused.
What problems surfaced when you tried resolving the values when requested (what you mention in the description)?
---
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: Add ManagementContext.getConfigWi...
Posted by rdowner <gi...@git.apache.org>.
Github user rdowner commented on a diff in the pull request:
https://github.com/apache/incubator-brooklyn/pull/1018#discussion_r44523173
--- Diff: core/src/main/java/org/apache/brooklyn/core/mgmt/internal/AbstractManagementContext.java ---
@@ -372,6 +377,27 @@ public BrooklynProperties getBrooklynProperties() {
return configMap;
}
+ public StringConfigMap getConfigWithExternalConfigResolved() {
+ final ExternalConfigSupplierRegistry externalConfigProviderRegistry = getExternalConfigProviderRegistry();
+ if (externalConfigProviderRegistry == null)
+ return configMap; // no external config available yet, so return config without resolving
--- End diff --
This check is probably superfluous. It was required in an earlier iteration of this code where the constructor had possibly not finished, but that should not be the case now. I will either remove it, or (to be safe) make it throw an `IllegalStateException`.
---
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: Add ManagementContext.getConfigWi...
Posted by ahgittin <gi...@git.apache.org>.
Github user ahgittin commented on the pull request:
https://github.com/apache/incubator-brooklyn/pull/1018#issuecomment-166279924
@rdowner can we close?
---
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: Add ManagementContext.getConfigWi...
Posted by aledsage <gi...@git.apache.org>.
Github user aledsage commented on the pull request:
https://github.com/apache/incubator-brooklyn/pull/1018#issuecomment-165162790
@neykov @rdowner I'm having a look at this - looking at neykov's suggestion.
---
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: Add ManagementContext.getConfigWi...
Posted by rdowner <gi...@git.apache.org>.
Github user rdowner commented on the pull request:
https://github.com/apache/incubator-brooklyn/pull/1018#issuecomment-169070502
Aled's implementation is better :-) closing this one.
---
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: Add ManagementContext.getConfigWi...
Posted by rdowner <gi...@git.apache.org>.
Github user rdowner commented on the pull request:
https://github.com/apache/incubator-brooklyn/pull/1018#issuecomment-155745923
@neykov I saw a problem if I modified `LocalManagementContext.getConfig()` to do the resolution. If this method returns something that is not the original `configMap` - even if it is an identical copy (such as by replacing the method body with `return BrooklynProperties.Factory.newEmpty().addFrom(configMap);`) then I got this exception during launch:
```
2015-11-11 11:10:20,534 WARN Error importing catalog from classpath:/brooklyn/default.catalog.bom: org.apache.brooklyn.util.exceptions.CompoundRuntimeException: Could not resolve item server, 2 errors including: Transformer for Brooklyn OASIS CAMP interpreter gave an error creating this plan: No CAMP Platform is registered with this Brooklyn management context.
org.apache.brooklyn.util.exceptions.CompoundRuntimeException: Could not resolve item server, 2 errors including: Transformer for Brooklyn OASIS CAMP interpreter gave an error creating this plan: No CAMP Platform is registered with this Brooklyn management context.
at org.apache.brooklyn.util.exceptions.Exceptions.create(Exceptions.java:306) ~[org.apache.brooklyn-brooklyn-utils-common-0.9.0-SNAPSHOT.jar:0.9.0-SNAPSHOT]
Caused by: java.lang.IllegalStateException: Transformer for Brooklyn OASIS CAMP interpreter gave an error creating this plan: No CAMP Platform is registered with this Brooklyn management context.
at org.apache.brooklyn.util.guava.IllegalStateExceptionSupplier.get(IllegalStateExceptionSupplier.java:52) ~[org.apache.brooklyn-brooklyn-utils-common-0.9.0-SNAPSHOT.jar:0.9.0-SNAPSHOT]
Caused by: org.apache.brooklyn.util.exceptions.PropagatedRuntimeException:
at org.apache.brooklyn.util.exceptions.Exceptions.create(Exceptions.java:298) ~[org.apache.brooklyn-brooklyn-utils-common-0.9.0-SNAPSHOT.jar:0.9.0-SNAPSHOT]
Caused by: org.apache.brooklyn.util.exceptions.PropagatedRuntimeException: Transformer for Brooklyn OASIS CAMP interpreter gave an error creating this plan: No CAMP Platform is registered with this Brooklyn management context.
at org.apache.brooklyn.core.plan.PlanToSpecFactory.attemptWithLoaders(PlanToSpecFactory.java:110) ~[org.apache.brooklyn-brooklyn-core-0.9.0-SNAPSHOT.jar:0.9.0-SNAPSHOT]
Caused by: java.lang.IllegalStateException: No CAMP Platform is registered with this Brooklyn management context.
at org.apache.brooklyn.camp.brooklyn.spi.creation.CampUtils.getCampPlatform(CampUtils.java:217) ~[org.apache.brooklyn-brooklyn-camp-0.9.0-SNAPSHOT.jar:0.9.0-SNAPSHOT]
```
Interestingly if I modify the method to use a MemoizedSupplier - meaning that it will consistently return the same object - the error disappears. This suggests that there is something that gets upset if `getConfig()` returns a different object, even if the contents are the same.
[A thought occurs to me. Is something *modifying* the config object that is returned?]
---
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: Add ManagementContext.getConfigWi...
Posted by neykov <gi...@git.apache.org>.
Github user neykov commented on a diff in the pull request:
https://github.com/apache/incubator-brooklyn/pull/1018#discussion_r44507157
--- Diff: core/src/main/java/org/apache/brooklyn/core/mgmt/internal/AbstractManagementContext.java ---
@@ -372,6 +377,27 @@ public BrooklynProperties getBrooklynProperties() {
return configMap;
}
+ public StringConfigMap getConfigWithExternalConfigResolved() {
+ final ExternalConfigSupplierRegistry externalConfigProviderRegistry = getExternalConfigProviderRegistry();
+ if (externalConfigProviderRegistry == null)
+ return configMap; // no external config available yet, so return config without resolving
--- End diff --
May be set external config values to null to prevent using the raw string or even throw as the caller would expect them to be resolved already?
---
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: Add ManagementContext.getConfigWi...
Posted by aledsage <gi...@git.apache.org>.
Github user aledsage commented on the pull request:
https://github.com/apache/incubator-brooklyn/pull/1018#issuecomment-165244406
@rdowner @neykov See https://github.com/apache/incubator-brooklyn/pull/1112, which gives an alternative implemention. I believe that replaces this PR, so this can be closed.
---
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: Add ManagementContext.getConfigWi...
Posted by neykov <gi...@git.apache.org>.
Github user neykov commented on the pull request:
https://github.com/apache/incubator-brooklyn/pull/1018#issuecomment-155780641
So we need to resolve external values at `getConfig` time, but at the same time allow mutations of the properties. What if you patch the config in the `AbstractManagementContext` constructor, but lazily - here's what I mean. In the constructor wrap the properties in a delegate, including a reference to the mgmt context. On `get`s check the value and if needed resolve. Alternatively convert any external values to suppliers, resolving on `get` (i.e. make `BrooklynProperties` handle suppliers).
It's obviously hackish to mutate the `ManagementContext.getConfig()` - might be a good reason to find a generic way to do it, but out of scope for this.
---
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: Add ManagementContext.getConfigWi...
Posted by rdowner <gi...@git.apache.org>.
Github user rdowner closed the pull request at:
https://github.com/apache/incubator-brooklyn/pull/1018
---
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.
---