You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@brooklyn.apache.org by alasdairhodge <gi...@git.apache.org> on 2015/07/27 11:16:04 UTC
[GitHub] incubator-brooklyn pull request: Externalised configuration
GitHub user alasdairhodge opened a pull request:
https://github.com/apache/incubator-brooklyn/pull/770
Externalised configuration
**Bare-bones implementation** of a proposed scheme for YAML blueprints to delegate selected configuration values to runtime-injected suppliers.
[Rationale, description and discussion in Google docs.](https://docs.google.com/document/d/1_cId6tVZ79ycmvn-DAmFBZDdtQxiHRm9sOilCfMigBc/edit#)
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/alasdairhodge/incubator-brooklyn externalised-config
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/incubator-brooklyn/pull/770.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 #770
----
commit 05b7aad1eaf031e764cd14c2c4d24cdcadb336b4
Author: Alasdair Hodge <gi...@alasdairhodge.co.uk>
Date: 2015-06-22T15:20:46Z
Initial types for external config suppliers, and registry of same.
commit f8153c580ab0f7b103d6c71c1856367b5940dbdb
Author: Alasdair Hodge <gi...@alasdairhodge.co.uk>
Date: 2015-06-22T15:50:33Z
Bootstrap external config suppliers from brooklyn properties.
commit 99be5b941e91c367f1053cc73ceb2f645b4b630f
Author: Alasdair Hodge <gi...@alasdairhodge.co.uk>
Date: 2015-06-22T15:52:28Z
Add external-config-supplier registry to management context.
commit 59c05681174943d4465d99ee2e6504c7b1a8bca3
Author: Alasdair Hodge <gi...@alasdairhodge.co.uk>
Date: 2015-06-22T15:53:14Z
Brooklyn YAML supports deferred value resolution via $brooklyn:external().
commit f9fd1dfb16dd3ae7e74841d21cc95e294d1d41b6
Author: Alasdair Hodge <gi...@alasdairhodge.co.uk>
Date: 2015-06-22T16:33:41Z
Added basic concrete ExternalConfigSupplier implementations.
commit 0a3b53dc32e4c7fd5e2747cd1350bf4b1e52b0bc
Author: Alasdair Hodge <gi...@alasdairhodge.co.uk>
Date: 2015-07-16T08:24:28Z
Make Brooklyn management context available to config suppliers.
----
---
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: Externalised configuration
Posted by alasdairhodge <gi...@git.apache.org>.
Github user alasdairhodge commented on a diff in the pull request:
https://github.com/apache/incubator-brooklyn/pull/770#discussion_r36412672
--- Diff: core/src/main/java/brooklyn/config/external/PropertiesFileExternalConfigSupplier.java ---
@@ -0,0 +1,68 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package brooklyn.config.external;
+
+import java.io.IOException;
+import java.io.InputStream;
+import java.net.URL;
+import java.util.Map;
+import java.util.Properties;
+
+import brooklyn.management.ManagementContext;
+import brooklyn.util.stream.Streams;
+
+
+/**
+ * Instances are populated from a plain java properties file named in the passed <code>config</code> map
+ * under the <code>propertiesUrl</code> key:
+ *
+ * <pre>
+ * brooklyn.external.foo = brooklyn.management.config.external.PropertiesFileExternalConfigSupplier
+ * brooklyn.external.foo.propertiesUrl = http://brooklyn.example.com/config/foo.properties
+ * </pre>
+ */
+public class PropertiesFileExternalConfigSupplier extends AbstractExternalConfigSupplier {
+
+ public static final String PROPERTIES_URL = "propertiesUrl";
+
+ private final Properties properties;
+
+ public PropertiesFileExternalConfigSupplier(ManagementContext managementContext, String name, Map<?, ?> config) throws IOException {
+ super(managementContext, name);
+ this.properties = loadProperties((String) config.get(PROPERTIES_URL));
--- End diff --
Yes, good suggestion. It could be a specific local mechanism (e.g. JMX operation), or we could extend the interface and wire it all back through Brooklyn; any preference?
---
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: Externalised configuration
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/770#discussion_r36255566
--- Diff: core/src/main/java/brooklyn/config/external/PropertiesFileExternalConfigSupplier.java ---
@@ -0,0 +1,68 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package brooklyn.config.external;
+
+import java.io.IOException;
+import java.io.InputStream;
+import java.net.URL;
+import java.util.Map;
+import java.util.Properties;
+
+import brooklyn.management.ManagementContext;
+import brooklyn.util.stream.Streams;
+
+
+/**
+ * Instances are populated from a plain java properties file named in the passed <code>config</code> map
+ * under the <code>propertiesUrl</code> key:
+ *
+ * <pre>
+ * brooklyn.external.foo = brooklyn.management.config.external.PropertiesFileExternalConfigSupplier
+ * brooklyn.external.foo.propertiesUrl = http://brooklyn.example.com/config/foo.properties
+ * </pre>
+ */
+public class PropertiesFileExternalConfigSupplier extends AbstractExternalConfigSupplier {
+
+ public static final String PROPERTIES_URL = "propertiesUrl";
+
+ private final Properties properties;
+
+ public PropertiesFileExternalConfigSupplier(ManagementContext managementContext, String name, Map<?, ?> config) throws IOException {
+ super(managementContext, name);
+ this.properties = loadProperties((String) config.get(PROPERTIES_URL));
--- End diff --
Should there be a way to force a reload of the properties?
---
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: Externalised configuration
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/770#discussion_r36255595
--- Diff: core/src/main/java/brooklyn/config/external/PropertiesFileExternalConfigSupplier.java ---
@@ -0,0 +1,68 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package brooklyn.config.external;
+
+import java.io.IOException;
+import java.io.InputStream;
+import java.net.URL;
+import java.util.Map;
+import java.util.Properties;
+
+import brooklyn.management.ManagementContext;
+import brooklyn.util.stream.Streams;
+
+
+/**
+ * Instances are populated from a plain java properties file named in the passed <code>config</code> map
+ * under the <code>propertiesUrl</code> key:
+ *
+ * <pre>
+ * brooklyn.external.foo = brooklyn.management.config.external.PropertiesFileExternalConfigSupplier
+ * brooklyn.external.foo.propertiesUrl = http://brooklyn.example.com/config/foo.properties
+ * </pre>
+ */
+public class PropertiesFileExternalConfigSupplier extends AbstractExternalConfigSupplier {
--- End diff --
Simple test case for this, and for `InPlaceExternalConfigSupplier`?
---
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: Externalised configuration
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/770#discussion_r36256497
--- Diff: core/src/main/java/brooklyn/management/internal/BasicExternalConfigSupplierRegistry.java ---
@@ -0,0 +1,108 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package brooklyn.management.internal;
+
+import java.lang.reflect.Constructor;
+import java.util.Map;
+
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import brooklyn.config.ConfigMap;
+import brooklyn.config.ConfigPredicates;
+import brooklyn.config.ConfigUtils;
+import brooklyn.config.external.ExternalConfigSupplier;
+import brooklyn.management.ManagementContext;
+
+import com.google.common.collect.Maps;
+
+/**
+ * Simple registry implementation.
+ *
+ * Permits a number of {@link ExternalConfigSupplier} instances to be registered, each with a unique name, for future
+ * (deferred) lookup of configuration values.
+ */
+public class BasicExternalConfigSupplierRegistry implements ExternalConfigSupplierRegistry {
+
+ private static final Logger LOG = LoggerFactory.getLogger(BasicExternalConfigSupplierRegistry.class);
+
+ private final Map<String, ExternalConfigSupplier> providersByName = Maps.newLinkedHashMap();
+
+ public BasicExternalConfigSupplierRegistry(ManagementContext mgmt) {
+ updateFromBrooklynProperties(mgmt);
+ }
+
+ synchronized public void addProvider(String name, ExternalConfigSupplier provider) {
+ if (providersByName.containsKey(name))
+ throw new IllegalArgumentException("Provider already registered with name '" + name + "'");
+ providersByName.put(name, provider);
+ }
+
+ synchronized public void removeProvider(String name) {
+ providersByName.remove(name);
+ }
+
+ /**
+ * Searches the named {@link ExternalConfigSupplier} for the config value associated with the specified key.
--- End diff --
Why repeat the implemented interface's method's javadoc verbatim?
Also, I'd include an `@Override` on the method.
---
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: Externalised configuration
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/770#discussion_r36256176
--- Diff: core/src/main/java/brooklyn/management/internal/BasicExternalConfigSupplierRegistry.java ---
@@ -0,0 +1,108 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package brooklyn.management.internal;
+
+import java.lang.reflect.Constructor;
+import java.util.Map;
+
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import brooklyn.config.ConfigMap;
+import brooklyn.config.ConfigPredicates;
+import brooklyn.config.ConfigUtils;
+import brooklyn.config.external.ExternalConfigSupplier;
+import brooklyn.management.ManagementContext;
+
+import com.google.common.collect.Maps;
+
+/**
+ * Simple registry implementation.
+ *
+ * Permits a number of {@link ExternalConfigSupplier} instances to be registered, each with a unique name, for future
+ * (deferred) lookup of configuration values.
+ */
+public class BasicExternalConfigSupplierRegistry implements ExternalConfigSupplierRegistry {
+
+ private static final Logger LOG = LoggerFactory.getLogger(BasicExternalConfigSupplierRegistry.class);
+
+ private final Map<String, ExternalConfigSupplier> providersByName = Maps.newLinkedHashMap();
+
+ public BasicExternalConfigSupplierRegistry(ManagementContext mgmt) {
+ updateFromBrooklynProperties(mgmt);
+ }
+
+ synchronized public void addProvider(String name, ExternalConfigSupplier provider) {
--- End diff --
Personal preference to not synchronize on `this`. I'd prefer synchronizing on a `private final Object` field. If you also want to support more advanced sub-classing, then can have a protected getter for the mutex.
---
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: Externalised configuration
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/770#discussion_r36255170
--- Diff: core/src/main/java/brooklyn/config/external/ExternalConfigSupplier.java ---
@@ -0,0 +1,29 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package brooklyn.config.external;
+
+/**
+ * Provider of "externalised" entity configuration that is resolved at runtime.
+ */
+public interface ExternalConfigSupplier {
--- End diff --
Do we want to include `@Beta`?
Worth including `@since 0.8.0` in the javadoc. Same for other classes that a user might touch.
---
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: Externalised configuration
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/770#discussion_r36256018
--- Diff: core/src/main/java/brooklyn/management/internal/BasicExternalConfigSupplierRegistry.java ---
@@ -0,0 +1,108 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package brooklyn.management.internal;
+
+import java.lang.reflect.Constructor;
+import java.util.Map;
+
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import brooklyn.config.ConfigMap;
+import brooklyn.config.ConfigPredicates;
+import brooklyn.config.ConfigUtils;
+import brooklyn.config.external.ExternalConfigSupplier;
+import brooklyn.management.ManagementContext;
+
+import com.google.common.collect.Maps;
+
+/**
+ * Simple registry implementation.
+ *
+ * Permits a number of {@link ExternalConfigSupplier} instances to be registered, each with a unique name, for future
+ * (deferred) lookup of configuration values.
+ */
+public class BasicExternalConfigSupplierRegistry implements ExternalConfigSupplierRegistry {
+
+ private static final Logger LOG = LoggerFactory.getLogger(BasicExternalConfigSupplierRegistry.class);
+
+ private final Map<String, ExternalConfigSupplier> providersByName = Maps.newLinkedHashMap();
+
+ public BasicExternalConfigSupplierRegistry(ManagementContext mgmt) {
+ updateFromBrooklynProperties(mgmt);
+ }
+
+ synchronized public void addProvider(String name, ExternalConfigSupplier provider) {
+ if (providersByName.containsKey(name))
+ throw new IllegalArgumentException("Provider already registered with name '" + name + "'");
+ providersByName.put(name, provider);
+ }
+
+ synchronized public void removeProvider(String name) {
+ providersByName.remove(name);
+ }
+
+ /**
+ * Searches the named {@link ExternalConfigSupplier} for the config value associated with the specified key.
+ * Quietly returns <code>null</code> if no config exists for the specified key.
+ * Throws {@link IllegalArgumentException} if no {@link ExternalConfigSupplier} exists for the passed name.
+ */
+ synchronized public String getConfig(String providerName, String key) {
+ ExternalConfigSupplier provider = providersByName.get(providerName);
+ if (provider == null)
+ throw new IllegalArgumentException("No provider found with name '" + providerName + "'");
+ return provider.get(key);
+ }
+
+ @SuppressWarnings("unchecked")
+ synchronized private void updateFromBrooklynProperties(ManagementContext mgmt) {
+ // form is:
+ // brooklyn.external.<name> : fully.qualified.ClassName
+ // brooklyn.external.<name>.<key> : <value>
+ // brooklyn.external.<name>.<key> : <value>
+ // brooklyn.external.<name>.<key> : <value>
+
+ String EXTERNAL_PROVIDER_PREFIX = "brooklyn.external.";
+ ConfigMap externalProviderProperties = mgmt.getConfig().submap(ConfigPredicates.startingWith(EXTERNAL_PROVIDER_PREFIX));
+ ClassLoader classloader = mgmt.getCatalogClassLoader();
+
+ for (String key : externalProviderProperties.asMapWithStringKeys().keySet()) {
+ String strippedKey = key.substring(EXTERNAL_PROVIDER_PREFIX.length());
+ if (strippedKey.contains("."))
+ continue;
+
+ String name = strippedKey;
+ String providerClassname = (String) externalProviderProperties.asMapWithStringKeys().get(key);
+ Map<String, Object> config = ConfigUtils.filterForPrefixAndStrip(externalProviderProperties.asMapWithStringKeys(), key+".");
+
+ try {
+ Class<? extends ExternalConfigSupplier> providerClass = (Class<? extends ExternalConfigSupplier>) classloader.loadClass(providerClassname);
+ Constructor<? extends ExternalConfigSupplier> constructor = providerClass.getConstructor(
+ ManagementContext.class, String.class, Map.class);
+ ExternalConfigSupplier configSupplier = constructor.newInstance(this, name, config);
+ addProvider(name, configSupplier);
+ LOG.info("Added external config supplier named '" + name + "': " + configSupplier);
+
+ } catch (Exception e) {
+ LOG.error("Failed to instantiate external config supplier named '" + name + "': " + e, e);
--- End diff --
Should we just log and continue? Or should we propagate? Given these values came from brooklyn.properties (rather than persisted state etc), then it feels reasonable to fail startup of the management context.
Thoughts?
---
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: Externalised configuration
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/incubator-brooklyn/pull/770
---
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: Externalised configuration
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/770#discussion_r36256288
--- Diff: core/src/main/java/brooklyn/config/external/PropertiesFileExternalConfigSupplier.java ---
@@ -0,0 +1,68 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package brooklyn.config.external;
+
+import java.io.IOException;
+import java.io.InputStream;
+import java.net.URL;
+import java.util.Map;
+import java.util.Properties;
+
+import brooklyn.management.ManagementContext;
+import brooklyn.util.stream.Streams;
+
+
+/**
+ * Instances are populated from a plain java properties file named in the passed <code>config</code> map
+ * under the <code>propertiesUrl</code> key:
+ *
+ * <pre>
+ * brooklyn.external.foo = brooklyn.management.config.external.PropertiesFileExternalConfigSupplier
+ * brooklyn.external.foo.propertiesUrl = http://brooklyn.example.com/config/foo.properties
+ * </pre>
+ */
+public class PropertiesFileExternalConfigSupplier extends AbstractExternalConfigSupplier {
+
+ public static final String PROPERTIES_URL = "propertiesUrl";
+
+ private final Properties properties;
+
+ public PropertiesFileExternalConfigSupplier(ManagementContext managementContext, String name, Map<?, ?> config) throws IOException {
--- End diff --
Ah, I see it's because this will be reflectively instantiated!
---
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: Externalised configuration
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/770#discussion_r36256972
--- Diff: core/src/main/java/brooklyn/config/external/ExternalConfigSupplier.java ---
@@ -0,0 +1,29 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package brooklyn.config.external;
+
+/**
+ * Provider of "externalised" entity configuration that is resolved at runtime.
+ */
+public interface ExternalConfigSupplier {
+
+ String getName();
+ String get(String key);
--- End diff --
As stated in your doc:
The supplier’s get()-semantics should be clearly described in the javadoc of the implementation class.
Can you add javadoc to that effect 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: Externalised configuration
Posted by aledsage <gi...@git.apache.org>.
Github user aledsage commented on the pull request:
https://github.com/apache/incubator-brooklyn/pull/770#issuecomment-127799442
A good start to the implementation of your doc (which I really like).
Quoting from your doc, will the impl achieve this (or is that still to be done):
Brooklyn will ensure that every request (i.e. call to entity.getConfig()) is delegated to the supplier, not just the first
Next step I think is unit tests.
---
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: Externalised configuration
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/770#discussion_r36255293
--- Diff: core/src/main/java/brooklyn/config/external/InPlaceExternalConfigSupplier.java ---
@@ -0,0 +1,51 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package brooklyn.config.external;
+
+import java.util.Map;
+
+import brooklyn.management.ManagementContext;
+
+
+/**
+ * Instances are populated via sub-keys specified directly in the <tt>brooklyn.properties</tt> file:
+ *
+ * <pre>
+ * brooklyn.external.foo = brooklyn.management.config.external.InPlaceConfigSupplier
+ * brooklyn.external.foo.key1 = value1
+ * brooklyn.external.foo.key2 = value2
+ * </pre>
+ *
+ * This will instantiate an <code>InPlaceExternalConfigSupplier</code> populated with values for <code>key1</code>
+ * and <code>key2</code>. Note that the <code>brooklyn.external.<name></code> prefix is stripped.
+ */
+public class InPlaceExternalConfigSupplier extends AbstractExternalConfigSupplier {
+
+ private final Map<?,?> config;
+
+ public InPlaceExternalConfigSupplier(ManagementContext managementContext, String name, Map<?, ?> config) {
--- End diff --
Should the constructor's map arg be `Map<String, String>`? We look up using string keys and we cast the returned value to String.
---
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: Externalised configuration
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/770#discussion_r36255506
--- Diff: core/src/main/java/brooklyn/config/external/PropertiesFileExternalConfigSupplier.java ---
@@ -0,0 +1,68 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package brooklyn.config.external;
+
+import java.io.IOException;
+import java.io.InputStream;
+import java.net.URL;
+import java.util.Map;
+import java.util.Properties;
+
+import brooklyn.management.ManagementContext;
+import brooklyn.util.stream.Streams;
+
+
+/**
+ * Instances are populated from a plain java properties file named in the passed <code>config</code> map
+ * under the <code>propertiesUrl</code> key:
+ *
+ * <pre>
+ * brooklyn.external.foo = brooklyn.management.config.external.PropertiesFileExternalConfigSupplier
+ * brooklyn.external.foo.propertiesUrl = http://brooklyn.example.com/config/foo.properties
+ * </pre>
+ */
+public class PropertiesFileExternalConfigSupplier extends AbstractExternalConfigSupplier {
+
+ public static final String PROPERTIES_URL = "propertiesUrl";
+
+ private final Properties properties;
+
+ public PropertiesFileExternalConfigSupplier(ManagementContext managementContext, String name, Map<?, ?> config) throws IOException {
--- End diff --
Why take a `Map<?, ?> config` in the constructor, rather than passing the properties URL as an explicit parameter (very groovy'ish!)? Is there a special way you envisage this being used?
---
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: Externalised configuration
Posted by ahgittin <gi...@git.apache.org>.
Github user ahgittin commented on the pull request:
https://github.com/apache/incubator-brooklyn/pull/770#issuecomment-136755818
Very useful. Test and documentation are needed, but I'll merge it.
---
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.
---