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 2016/11/16 12:37:52 UTC
[GitHub] brooklyn-server pull request #440: BROOKLYN-345: persist brooklyn.parameters...
GitHub user aledsage opened a pull request:
https://github.com/apache/brooklyn-server/pull/440
BROOKLYN-345: persist brooklyn.parameters (fixes ServiceFailureDetector rebind)
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/aledsage/brooklyn-server BROOKLYN-345
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/brooklyn-server/pull/440.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 #440
----
commit 31d84a69cdc66efef7a0f064789a0d4d1693e8e4
Author: Aled Sage <al...@gmail.com>
Date: 2016-11-16T09:47:27Z
Time.makeTimeString: avoid NPE if arg is null
commit 53f6b1379ac8d50250564b17d9ada2a6c1b661d2
Author: Aled Sage <al...@gmail.com>
Date: 2016-11-16T09:51:43Z
ServiceFailureDetector yaml tests, and fix
commit 6e8b7b4d927fc3d152753fe673957c7854705589
Author: Aled Sage <al...@gmail.com>
Date: 2016-11-16T09:58:08Z
BROOKLYN-345: add (failing) tests for rebind
commit f7026a792c6b7007ea38971cd51b599334abc56c
Author: Aled Sage <al...@gmail.com>
Date: 2016-11-16T09:59:46Z
ServiceFailureDetector: persist lastPublished
commit b530cd670883bb7a901c72ecfa79692670b5fb08
Author: Aled Sage <al...@gmail.com>
Date: 2016-11-16T12:34:07Z
BROOKLYN-345: Persist entity\u2019s dynamic config keys
----
---
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] brooklyn-server issue #440: BROOKLYN-345: persist brooklyn.parameters (fixes...
Posted by aledsage <gi...@git.apache.org>.
Github user aledsage commented on the issue:
https://github.com/apache/brooklyn-server/pull/440
Thanks @neykov - `ConfigNestedYamlTest.testCatalogParameterFromSuperYamlTypeAsSoftware` is a very annoying and valid failure!
There is a `MapConfigKey` defined on the java type (`SoftwareProcess.TEMPLATE_SUBSTITUTIONS`). In the yaml (`config-nested-tests,bom`), the super-type declares a brooklyn.parameters for this: brooklyn.parameters: [ {name: template.substitutions, type: java.util.Map} ]`.
Note the difference in type for the config keys: the first is a `MapConfigKey` and the latter is a `BasicConfigKey<java.util.Map>`.
When the config values are written into the entity's spec, it uses the `MapConfigKey` so the value of `{field: val}` is actually written as `{template.substitutions.field: val}`.
When the config value is later read, it uses the `BasicConfigKey<java.util.Map>`, so it just looks up `template.substitutions` and thus returns an empty map.
---
A few things spring to mind:
1. don't try to change the type of a config key when you're overriding it in brooklyn.parameters. (However, the user doesn't think they've done this - it's of type map!)
2. write creating the spec, use the same config key as will subsequently be used for reading the value back out again.
3. I really don't like the `MapConfigKey` stuff. Particularly now that we have capabilities for defining the merge-semantics of map config keys, the need for the `MapConfigKey` is greatly reduced.
Let's not try to tackle (3) right now, but that is one for the mailing list!
(2) feels like the right thing to do. But it's fiddly. We're in the middle of the stacktrace below for (one of the places?) where we're writing the config value using the `MapConfigKey`:
```
"main" prio=5 tid=0x00007fdbab000000 nid=0x1703 at breakpoint[0x0000700000217000]
java.lang.Thread.State: RUNNABLE
at org.apache.brooklyn.api.internal.AbstractBrooklynObjectSpec.configure(AbstractBrooklynObjectSpec.java:293)
at org.apache.brooklyn.camp.brooklyn.spi.creation.BrooklynComponentTemplateResolver.configureEntityConfig(BrooklynComponentTemplateResolver.java:337)
at org.apache.brooklyn.camp.brooklyn.spi.creation.BrooklynComponentTemplateResolver.decorateSpec(BrooklynComponentTemplateResolver.java:259)
at org.apache.brooklyn.camp.brooklyn.spi.creation.BrooklynComponentTemplateResolver.populateSpec(BrooklynComponentTemplateResolver.java:250)
at org.apache.brooklyn.camp.brooklyn.spi.creation.BrooklynComponentTemplateResolver.resolveSpec(BrooklynComponentTemplateResolver.java:189)
at org.apache.brooklyn.camp.brooklyn.spi.creation.BrooklynAssemblyTemplateInstantiator.buildTemplateServicesAsSpecs(BrooklynAssemblyTemplateInstantiator.java:114)
at org.apache.brooklyn.camp.brooklyn.spi.creation.BrooklynAssemblyTemplateInstantiator.createServiceSpecs(BrooklynAssemblyTemplateInstantiator.java:73)
at org.apache.brooklyn.camp.brooklyn.spi.creation.BrooklynAssemblyTemplateInstantiator.createApplicationSpec(BrooklynAssemblyTemplateInstantiator.java:90)
at org.apache.brooklyn.camp.brooklyn.spi.creation.CampResolver.createEntitySpecFromServicesBlock(CampResolver.java:145)
at org.apache.brooklyn.camp.brooklyn.spi.creation.CampResolver.createSpecFromFull(CampResolver.java:111)
at org.apache.brooklyn.camp.brooklyn.spi.creation.CampToSpecTransformer.createCatalogSpec(CampToSpecTransformer.java:102)
at org.apache.brooklyn.core.catalog.internal.BasicBrooklynCatalog$1.apply(BasicBrooklynCatalog.java:331)
at org.apache.brooklyn.core.catalog.internal.BasicBrooklynCatalog$1.apply(BasicBrooklynCatalog.java:1)
at org.apache.brooklyn.core.plan.PlanToSpecFactory.attemptWithLoaders(PlanToSpecFactory.java:126)
at org.apache.brooklyn.core.plan.PlanToSpecFactory.attemptWithLoaders(PlanToSpecFactory.java:118)
at org.apache.brooklyn.core.catalog.internal.BasicBrooklynCatalog.internalCreateSpecLegacy(BasicBrooklynCatalog.java:328)
at org.apache.brooklyn.core.catalog.internal.BasicBrooklynCatalog$PlanInterpreterGuessingType.attemptType(BasicBrooklynCatalog.java:808)
at org.apache.brooklyn.core.catalog.internal.BasicBrooklynCatalog$PlanInterpreterGuessingType.reconstruct(BasicBrooklynCatalog.java:735)
at org.apache.brooklyn.core.catalog.internal.BasicBrooklynCatalog.collectCatalogItems(BasicBrooklynCatalog.java:508)
at org.apache.brooklyn.core.catalog.internal.BasicBrooklynCatalog.collectCatalogItems(BasicBrooklynCatalog.java:476)
at org.apache.brooklyn.core.catalog.internal.BasicBrooklynCatalog.collectCatalogItems(BasicBrooklynCatalog.java:383)
at org.apache.brooklyn.core.catalog.internal.BasicBrooklynCatalog.collectCatalogItems(BasicBrooklynCatalog.java:372)
at org.apache.brooklyn.core.catalog.internal.BasicBrooklynCatalog.addItems(BasicBrooklynCatalog.java:929)
at org.apache.brooklyn.core.catalog.internal.BasicBrooklynCatalog.addItems(BasicBrooklynCatalog.java:1)
at org.apache.brooklyn.camp.brooklyn.AbstractYamlTest.addCatalogItems(AbstractYamlTest.java:199)
at org.apache.brooklyn.camp.brooklyn.ConfigNestedYamlTest.testCatalogParameterFromSuperYamlTypeAsSoftware(ConfigNestedYamlTest.java:89)
```
Maybe this is one to document and defer? 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] brooklyn-server pull request #440: BROOKLYN-345: persist brooklyn.parameters...
Posted by neykov <gi...@git.apache.org>.
Github user neykov commented on a diff in the pull request:
https://github.com/apache/brooklyn-server/pull/440#discussion_r88249200
--- Diff: camp/camp-brooklyn/src/test/java/org/apache/brooklyn/camp/brooklyn/ServiceFailureDetectorYamlTest.java ---
@@ -0,0 +1,189 @@
+/*
+ * 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 org.apache.brooklyn.camp.brooklyn;
+
+import static org.testng.Assert.assertEquals;
+import static org.testng.Assert.assertNotNull;
+
+import java.util.Map;
+
+import org.apache.brooklyn.api.entity.Entity;
+import org.apache.brooklyn.api.sensor.Enricher;
+import org.apache.brooklyn.config.ConfigKey;
+import org.apache.brooklyn.core.entity.RecordingSensorEventListener;
+import org.apache.brooklyn.core.entity.lifecycle.ServiceStateLogic.ServiceNotUpLogic;
+import org.apache.brooklyn.core.sensor.SensorEventPredicates;
+import org.apache.brooklyn.core.test.entity.TestEntity;
+import org.apache.brooklyn.policy.ha.HASensors;
+import org.apache.brooklyn.policy.ha.ServiceFailureDetector;
+import org.apache.brooklyn.util.time.Duration;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+import org.testng.annotations.Test;
+
+import com.google.common.base.Joiner;
+import com.google.common.base.Predicates;
+import com.google.common.collect.ImmutableMap;
+import com.google.common.collect.Iterables;
+
+@Test
+public class ServiceFailureDetectorYamlTest extends AbstractYamlTest {
+
+ @SuppressWarnings("unused")
+ private static final Logger log = LoggerFactory.getLogger(ServiceFailureDetectorYamlTest.class);
+
+ static final String INDICATOR_KEY_1 = "test-indicator-1";
+
+ static final String appId = "my-app";
+ static final String appVersion = "1.2.3";
+ static final String appVersionedId = appId + ":" + appVersion;
+
+ static final String catalogYamlSimple = Joiner.on("\n").join(
+ "brooklyn.catalog:",
+ " id: " + appId,
+ " version: " + appVersion,
+ " itemType: entity",
+ " item:",
+ " type: " + TestEntity.class.getName(),
+ " brooklyn.enrichers:",
+ " - type: " + ServiceFailureDetector.class.getName());
+
+ static final String catalogYamlWithDsl = Joiner.on("\n").join(
+ "brooklyn.catalog:",
+ " id: my-app",
+ " version: 1.2.3",
+ " itemType: entity",
+ " item:",
+ " services:",
+ " - type: " + TestEntity.class.getName(),//FailingEntity.class.getName(),
+ " brooklyn.parameters:",
+ " - name: custom.stabilizationDelay",
+ " type: " + Duration.class.getName(),
+ " default: 1ms",
+ " - name: custom.republishTime",
+ " type: " + Duration.class.getName(),
+ " default: 1m",
+ " brooklyn.enrichers:",
+ " - type: " + ServiceFailureDetector.class.getName(),
+ " brooklyn.config:",
+ " serviceOnFire.stabilizationDelay: $brooklyn:config(\"custom.stabilizationDelay\")",
+ " entityFailed.stabilizationDelay: $brooklyn:config(\"custom.stabilizationDelay\")",
+ " entityRecovered.stabilizationDelay: $brooklyn:config(\"custom.stabilizationDelay\")",
+ " entityFailed.republishTime: $brooklyn:config(\"custom.republishTime\")");
+
+ static final String catalogYamlWithDslReferenceParentDefault = Joiner.on("\n").join(
+ "brooklyn.catalog:",
+ " id: my-app",
+ " version: 1.2.3",
+ " itemType: entity",
+ " item:",
+ " brooklyn.parameters:",
+ " - name: custom.stabilizationDelay",
+ " type: " + Duration.class.getName(),
+ " default: 1ms",
+ " - name: custom.republishTime",
+ " type: " + Duration.class.getName(),
+ " default: 1m",
+ " services:",
+ " - type: " + TestEntity.class.getName(),//FailingEntity.class.getName(),
+ " brooklyn.enrichers:",
+ " - type: " + ServiceFailureDetector.class.getName(),
+ " brooklyn.config:",
+ " serviceOnFire.stabilizationDelay: $brooklyn:config(\"custom.stabilizationDelay\")",
+ " entityFailed.stabilizationDelay: $brooklyn:config(\"custom.stabilizationDelay\")",
+ " entityRecovered.stabilizationDelay: $brooklyn:config(\"custom.stabilizationDelay\")",
+ " entityFailed.republishTime: $brooklyn:config(\"custom.republishTime\")");
--- End diff --
How does this even work? Default values are not inherited.
It's probably because the root parameters are merged to the top-level entity on unwrap. So in the end it's identical to the above test case.
---
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] brooklyn-server issue #440: BROOKLYN-345: persist brooklyn.parameters (fixes...
Posted by neykov <gi...@git.apache.org>.
Github user neykov commented on the issue:
https://github.com/apache/brooklyn-server/pull/440
Compile time errors in the jenkins build:
```
[ERROR] /home/jenkins/jenkins-slave/workspace/brooklyn-server-pull-requests/camp/camp-brooklyn/src/test/java/org/apache/brooklyn/camp/brooklyn/ServiceFailureDetectorYamlTest.java:[31,39] cannot find symbol
symbol: class SensorEventPredicates
location: package org.apache.brooklyn.core.sensor
```
---
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] brooklyn-server pull request #440: BROOKLYN-345: persist brooklyn.parameters...
Posted by ahgittin <gi...@git.apache.org>.
Github user ahgittin commented on a diff in the pull request:
https://github.com/apache/brooklyn-server/pull/440#discussion_r90607484
--- Diff: camp/camp-brooklyn/src/main/java/org/apache/brooklyn/camp/brooklyn/spi/creation/BrooklynComponentTemplateResolver.java ---
@@ -374,15 +374,45 @@ protected ConfigInheritance getDefaultConfigInheritance() {
* Searches for config keys in the type, additional interfaces and the implementation (if specified)
*/
private Collection<FlagConfigKeyAndValueRecord> findAllFlagsAndConfigKeyValues(EntitySpec<?> spec, ConfigBag bagFlags) {
+ // Matches the bagFlags against the names used in brooklyn.parameters, entity configKeys
+ // and entity fields with `@SetFromFlag`.
+ //
+ // Returns all config keys / flags that match things in bagFlags, including duplicates.
+ // For example, if a configKey in Java is re-declared in YAML `brooklyn.parameters`,
+ // then we'll get two records.
+ //
+ // Make some effort to have these returned in the right order. That is very important
+ // because they are added to the `EntitySpec.configure(key, val)`. If there is already
+ // a key in `EntitySpec.config`, then the put will replace the value and leave the key
+ // as-is (so the default-value and description of the key will remain as whatever the
+ // first put said).
+
+ // TODO We should remove duplicates, rather than just doing the `put` multiple times,
+ // relying on ordering. We should also respect the ordered returned by
+ // EntityDynamicType.getConfigKeys, which is much better (it respects `BasicConfigKeyOverwriting`
+ // etc).
+ //
+ // However, that is hard/fiddly because of the way a config key can be referenced by
+ // its real name or flag-name.
+ //
+ // I wonder if this is fundamentally broken (and I really do dislike our informal use
+ // of aliases). Consider a configKey with name A and alias B. The bagFlags could have
+ // {A: val1, B: val2}. There is no formal definition of which takes precedence. We'll add
+ // both to the entity's configBag, without any warning - it's up to the `config().get()`
+ // method to then figure out what to do. It gets worse if there is also a ConfigKey with
+ // name "B" the "val2" then applies to both!
--- End diff --
added above as a code comment in #462
---
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] brooklyn-server issue #440: BROOKLYN-345: persist brooklyn.parameters (fixes...
Posted by ahgittin <gi...@git.apache.org>.
Github user ahgittin commented on the issue:
https://github.com/apache/brooklyn-server/pull/440
i'll have a look at this ... likely some conflicts with https://github.com/apache/brooklyn-server/pull/462
---
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] brooklyn-server pull request #440: BROOKLYN-345: persist brooklyn.parameters...
Posted by ahgittin <gi...@git.apache.org>.
Github user ahgittin commented on a diff in the pull request:
https://github.com/apache/brooklyn-server/pull/440#discussion_r91068402
--- Diff: core/src/main/java/org/apache/brooklyn/core/config/internal/AbstractConfigMapImpl.java ---
@@ -163,13 +163,18 @@ public ConfigBag getLocalConfigBag() {
return putAllOwnConfigIntoSafely(ConfigBag.newInstance()).seal();
}
- public Object setConfig(ConfigKey<?> key, Object v) {
- Object val = coerceConfigVal(key, v);
+ public Object setConfig(final ConfigKey<?> key, Object v) {
+ // Use our own key for writing, (e.g. in-case it should (or should not) be a structured key like MapConfigKey).
+ // This is same logic as for getConfig, except we only have to look at our own container.
+ ConfigKey<?> ownKey = getKeyAtContainer(getContainer(), key);
+ if (ownKey==null) ownKey = key;
+
+ Object val = coerceConfigVal(ownKey, v);
--- End diff --
that's true @neykov re putting with a key with a different type to what you get with. but it would be a weird thing to do to put with an incompatible key (normally we'll put with the same key or one which uses a supertype eg `Object`) and not surprising if it's coerced to int on get then double on put. given that this has been the behaviour, and catching errors early is useful, i definitely don't see a compelling reason to change.
---
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] brooklyn-server issue #440: BROOKLYN-345: persist brooklyn.parameters (fixes...
Posted by neykov <gi...@git.apache.org>.
Github user neykov commented on the issue:
https://github.com/apache/brooklyn-server/pull/440
Finished review. Happy to merge after resolving the "lost default overrides" problem.
---
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] brooklyn-server pull request #440: BROOKLYN-345: persist brooklyn.parameters...
Posted by neykov <gi...@git.apache.org>.
Github user neykov commented on a diff in the pull request:
https://github.com/apache/brooklyn-server/pull/440#discussion_r89962290
--- Diff: camp/camp-brooklyn/src/main/java/org/apache/brooklyn/camp/brooklyn/spi/creation/BrooklynComponentTemplateResolver.java ---
@@ -374,15 +374,45 @@ protected ConfigInheritance getDefaultConfigInheritance() {
* Searches for config keys in the type, additional interfaces and the implementation (if specified)
*/
private Collection<FlagConfigKeyAndValueRecord> findAllFlagsAndConfigKeyValues(EntitySpec<?> spec, ConfigBag bagFlags) {
+ // Matches the bagFlags against the names used in brooklyn.parameters, entity configKeys
+ // and entity fields with `@SetFromFlag`.
+ //
+ // Returns all config keys / flags that match things in bagFlags, including duplicates.
+ // For example, if a configKey in Java is re-declared in YAML `brooklyn.parameters`,
+ // then we'll get two records.
+ //
+ // Make some effort to have these returned in the right order. That is very important
+ // because they are added to the `EntitySpec.configure(key, val)`. If there is already
+ // a key in `EntitySpec.config`, then the put will replace the value and leave the key
+ // as-is (so the default-value and description of the key will remain as whatever the
+ // first put said).
+
+ // TODO We should remove duplicates, rather than just doing the `put` multiple times,
+ // relying on ordering. We should also respect the ordered returned by
+ // EntityDynamicType.getConfigKeys, which is much better (it respects `BasicConfigKeyOverwriting`
+ // etc).
+ //
+ // However, that is hard/fiddly because of the way a config key can be referenced by
+ // its real name or flag-name.
+ //
+ // I wonder if this is fundamentally broken (and I really do dislike our informal use
+ // of aliases). Consider a configKey with name A and alias B. The bagFlags could have
+ // {A: val1, B: val2}. There is no formal definition of which takes precedence. We'll add
+ // both to the entity's configBag, without any warning - it's up to the `config().get()`
+ // method to then figure out what to do. It gets worse if there is also a ConfigKey with
+ // name "B" the "val2" then applies to both!
+ //
+ // I plan to propose a change on dev@brooklyn, to replace `@SetFromFlag`!
+
Set<FlagConfigKeyAndValueRecord> allKeys = MutableSet.of();
--- End diff --
Change to list to make intent clear. `FlagConfigKeyAndValueRecord` doesn't implement `equals`, `hash`.
---
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] brooklyn-server pull request #440: BROOKLYN-345: persist brooklyn.parameters...
Posted by ahgittin <gi...@git.apache.org>.
Github user ahgittin commented on a diff in the pull request:
https://github.com/apache/brooklyn-server/pull/440#discussion_r90607451
--- Diff: camp/camp-brooklyn/src/main/java/org/apache/brooklyn/camp/brooklyn/spi/creation/BrooklynComponentTemplateResolver.java ---
@@ -374,15 +374,45 @@ protected ConfigInheritance getDefaultConfigInheritance() {
* Searches for config keys in the type, additional interfaces and the implementation (if specified)
*/
private Collection<FlagConfigKeyAndValueRecord> findAllFlagsAndConfigKeyValues(EntitySpec<?> spec, ConfigBag bagFlags) {
+ // Matches the bagFlags against the names used in brooklyn.parameters, entity configKeys
+ // and entity fields with `@SetFromFlag`.
+ //
+ // Returns all config keys / flags that match things in bagFlags, including duplicates.
+ // For example, if a configKey in Java is re-declared in YAML `brooklyn.parameters`,
+ // then we'll get two records.
+ //
+ // Make some effort to have these returned in the right order. That is very important
+ // because they are added to the `EntitySpec.configure(key, val)`. If there is already
+ // a key in `EntitySpec.config`, then the put will replace the value and leave the key
+ // as-is (so the default-value and description of the key will remain as whatever the
+ // first put said).
+
+ // TODO We should remove duplicates, rather than just doing the `put` multiple times,
+ // relying on ordering. We should also respect the ordered returned by
+ // EntityDynamicType.getConfigKeys, which is much better (it respects `BasicConfigKeyOverwriting`
+ // etc).
+ //
+ // However, that is hard/fiddly because of the way a config key can be referenced by
+ // its real name or flag-name.
+ //
+ // I wonder if this is fundamentally broken (and I really do dislike our informal use
+ // of aliases). Consider a configKey with name A and alias B. The bagFlags could have
+ // {A: val1, B: val2}. There is no formal definition of which takes precedence. We'll add
+ // both to the entity's configBag, without any warning - it's up to the `config().get()`
+ // method to then figure out what to do. It gets worse if there is also a ConfigKey with
+ // name "B" the "val2" then applies to both!
+ //
+ // I plan to propose a change on dev@brooklyn, to replace `@SetFromFlag`!
+
Set<FlagConfigKeyAndValueRecord> allKeys = MutableSet.of();
--- End diff --
agree -- applying in #462
---
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] brooklyn-server issue #440: BROOKLYN-345: persist brooklyn.parameters (fixes...
Posted by aledsage <gi...@git.apache.org>.
Github user aledsage commented on the issue:
https://github.com/apache/brooklyn-server/pull/440
The latest changes (particularly for the commit `entity.setConfig(key,v): lookup ownKey`) are motivated by fixing the tests that failed after the config-param override fix. Investigating that led to fixing https://issues.apache.org/jira/browse/BROOKLYN-328.
I talked through these changes with @neykov before making them, so hopefully it's not too controversial!
---
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] brooklyn-server pull request #440: BROOKLYN-345: persist brooklyn.parameters...
Posted by ahgittin <gi...@git.apache.org>.
Github user ahgittin commented on a diff in the pull request:
https://github.com/apache/brooklyn-server/pull/440#discussion_r90010996
--- Diff: core/src/main/java/org/apache/brooklyn/core/config/internal/AbstractConfigMapImpl.java ---
@@ -163,13 +163,18 @@ public ConfigBag getLocalConfigBag() {
return putAllOwnConfigIntoSafely(ConfigBag.newInstance()).seal();
}
- public Object setConfig(ConfigKey<?> key, Object v) {
- Object val = coerceConfigVal(key, v);
+ public Object setConfig(final ConfigKey<?> key, Object v) {
+ // Use our own key for writing, (e.g. in-case it should (or should not) be a structured key like MapConfigKey).
+ // This is same logic as for getConfig, except we only have to look at our own container.
+ ConfigKey<?> ownKey = getKeyAtContainer(getContainer(), key);
+ if (ownKey==null) ownKey = key;
+
+ Object val = coerceConfigVal(ownKey, v);
--- End diff --
is there any harm in coercing best-effort on config puts additionally?
---
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] brooklyn-server pull request #440: BROOKLYN-345: persist brooklyn.parameters...
Posted by ahgittin <gi...@git.apache.org>.
Github user ahgittin commented on a diff in the pull request:
https://github.com/apache/brooklyn-server/pull/440#discussion_r90009674
--- Diff: camp/camp-brooklyn/src/main/java/org/apache/brooklyn/camp/brooklyn/spi/creation/BrooklynComponentTemplateResolver.java ---
@@ -374,15 +374,45 @@ protected ConfigInheritance getDefaultConfigInheritance() {
* Searches for config keys in the type, additional interfaces and the implementation (if specified)
*/
private Collection<FlagConfigKeyAndValueRecord> findAllFlagsAndConfigKeyValues(EntitySpec<?> spec, ConfigBag bagFlags) {
+ // Matches the bagFlags against the names used in brooklyn.parameters, entity configKeys
+ // and entity fields with `@SetFromFlag`.
+ //
+ // Returns all config keys / flags that match things in bagFlags, including duplicates.
+ // For example, if a configKey in Java is re-declared in YAML `brooklyn.parameters`,
+ // then we'll get two records.
+ //
+ // Make some effort to have these returned in the right order. That is very important
+ // because they are added to the `EntitySpec.configure(key, val)`. If there is already
+ // a key in `EntitySpec.config`, then the put will replace the value and leave the key
+ // as-is (so the default-value and description of the key will remain as whatever the
+ // first put said).
+
+ // TODO We should remove duplicates, rather than just doing the `put` multiple times,
+ // relying on ordering. We should also respect the ordered returned by
+ // EntityDynamicType.getConfigKeys, which is much better (it respects `BasicConfigKeyOverwriting`
+ // etc).
+ //
+ // However, that is hard/fiddly because of the way a config key can be referenced by
+ // its real name or flag-name.
+ //
+ // I wonder if this is fundamentally broken (and I really do dislike our informal use
+ // of aliases). Consider a configKey with name A and alias B. The bagFlags could have
+ // {A: val1, B: val2}. There is no formal definition of which takes precedence. We'll add
+ // both to the entity's configBag, without any warning - it's up to the `config().get()`
+ // method to then figure out what to do. It gets worse if there is also a ConfigKey with
+ // name "B" the "val2" then applies to both!
--- End diff --
I think things should be de-aliased when writing to the config map, so config map has canonical config key names _only_ -- ie it should not be up to `config().get()` to figure out if any aliases are in effect.
Is this not the case?
Agree if we have `{A: val1, B: val2 }` as flags where `A` and `B` are names/aliases for a given config key there is ambiguity -- this is possible even with `{A: val1, A: val2 }`. I'm not too worried about either of these, fair enough to say it's not guaranteed which is taken.
BTW good code change to treat the parameter definitions of config keys as the preferred ones. The only tweak I might suggest is that if the parameter fields are incomplete they might be merged with those defined on the type (eg description, default value) so that it isn't necessary to re-declare them as parameters.
---
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] brooklyn-server issue #440: BROOKLYN-345: persist brooklyn.parameters (fixes...
Posted by aledsage <gi...@git.apache.org>.
Github user aledsage commented on the issue:
https://github.com/apache/brooklyn-server/pull/440
The problem that I'm looking at fixing for `ConfigNestedYamlTest.testCatalogParameterFromSuperYamlTypeAsSoftware` should also fix https://issues.apache.org/jira/browse/BROOKLYN-328.
---
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] brooklyn-server issue #440: BROOKLYN-345: persist brooklyn.parameters (fixes...
Posted by neykov <gi...@git.apache.org>.
Github user neykov commented on the issue:
https://github.com/apache/brooklyn-server/pull/440
Failing jenkins tests:
```
Failed tests:
ConfigNestedYamlTest.testCatalogParameterFromSuperYamlTypeAsSoftware:90->doTestWithBlueprint:101->checkEntity:122 missing 'field' in {} expected [true] but found [false]
org.apache.brooklyn.camp.brooklyn.catalog.SpecParameterUnwrappingTest.testDependantCatalogMergesParameters(org.apache.brooklyn.camp.brooklyn.catalog.SpecParameterUnwrappingTest)
Run 1: SpecParameterUnwrappingTest.testDependantCatalogMergesParameters:247 expected [4] but found [3]
Run 2: SpecParameterUnwrappingTest.testDependantCatalogMergesParameters:250 expected [3] but found [2]
Run 3: SpecParameterUnwrappingTest.testDependantCatalogMergesParameters:253 expected [8] but found [7]
```
---
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] brooklyn-server pull request #440: BROOKLYN-345: persist brooklyn.parameters...
Posted by aledsage <gi...@git.apache.org>.
Github user aledsage commented on a diff in the pull request:
https://github.com/apache/brooklyn-server/pull/440#discussion_r88331232
--- Diff: camp/camp-brooklyn/src/test/java/org/apache/brooklyn/camp/brooklyn/ServiceFailureDetectorYamlTest.java ---
@@ -0,0 +1,189 @@
+/*
+ * 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 org.apache.brooklyn.camp.brooklyn;
+
+import static org.testng.Assert.assertEquals;
+import static org.testng.Assert.assertNotNull;
+
+import java.util.Map;
+
+import org.apache.brooklyn.api.entity.Entity;
+import org.apache.brooklyn.api.sensor.Enricher;
+import org.apache.brooklyn.config.ConfigKey;
+import org.apache.brooklyn.core.entity.RecordingSensorEventListener;
+import org.apache.brooklyn.core.entity.lifecycle.ServiceStateLogic.ServiceNotUpLogic;
+import org.apache.brooklyn.core.sensor.SensorEventPredicates;
+import org.apache.brooklyn.core.test.entity.TestEntity;
+import org.apache.brooklyn.policy.ha.HASensors;
+import org.apache.brooklyn.policy.ha.ServiceFailureDetector;
+import org.apache.brooklyn.util.time.Duration;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+import org.testng.annotations.Test;
+
+import com.google.common.base.Joiner;
+import com.google.common.base.Predicates;
+import com.google.common.collect.ImmutableMap;
+import com.google.common.collect.Iterables;
+
+@Test
+public class ServiceFailureDetectorYamlTest extends AbstractYamlTest {
+
+ @SuppressWarnings("unused")
+ private static final Logger log = LoggerFactory.getLogger(ServiceFailureDetectorYamlTest.class);
+
+ static final String INDICATOR_KEY_1 = "test-indicator-1";
+
+ static final String appId = "my-app";
+ static final String appVersion = "1.2.3";
+ static final String appVersionedId = appId + ":" + appVersion;
+
+ static final String catalogYamlSimple = Joiner.on("\n").join(
+ "brooklyn.catalog:",
+ " id: " + appId,
+ " version: " + appVersion,
+ " itemType: entity",
+ " item:",
+ " type: " + TestEntity.class.getName(),
+ " brooklyn.enrichers:",
+ " - type: " + ServiceFailureDetector.class.getName());
+
+ static final String catalogYamlWithDsl = Joiner.on("\n").join(
+ "brooklyn.catalog:",
+ " id: my-app",
+ " version: 1.2.3",
+ " itemType: entity",
+ " item:",
+ " services:",
+ " - type: " + TestEntity.class.getName(),//FailingEntity.class.getName(),
+ " brooklyn.parameters:",
+ " - name: custom.stabilizationDelay",
+ " type: " + Duration.class.getName(),
+ " default: 1ms",
+ " - name: custom.republishTime",
+ " type: " + Duration.class.getName(),
+ " default: 1m",
+ " brooklyn.enrichers:",
+ " - type: " + ServiceFailureDetector.class.getName(),
+ " brooklyn.config:",
+ " serviceOnFire.stabilizationDelay: $brooklyn:config(\"custom.stabilizationDelay\")",
+ " entityFailed.stabilizationDelay: $brooklyn:config(\"custom.stabilizationDelay\")",
+ " entityRecovered.stabilizationDelay: $brooklyn:config(\"custom.stabilizationDelay\")",
+ " entityFailed.republishTime: $brooklyn:config(\"custom.republishTime\")");
+
+ static final String catalogYamlWithDslReferenceParentDefault = Joiner.on("\n").join(
+ "brooklyn.catalog:",
+ " id: my-app",
+ " version: 1.2.3",
+ " itemType: entity",
+ " item:",
+ " brooklyn.parameters:",
+ " - name: custom.stabilizationDelay",
+ " type: " + Duration.class.getName(),
+ " default: 1ms",
+ " - name: custom.republishTime",
+ " type: " + Duration.class.getName(),
+ " default: 1m",
+ " services:",
+ " - type: " + TestEntity.class.getName(),//FailingEntity.class.getName(),
+ " brooklyn.enrichers:",
+ " - type: " + ServiceFailureDetector.class.getName(),
+ " brooklyn.config:",
+ " serviceOnFire.stabilizationDelay: $brooklyn:config(\"custom.stabilizationDelay\")",
+ " entityFailed.stabilizationDelay: $brooklyn:config(\"custom.stabilizationDelay\")",
+ " entityRecovered.stabilizationDelay: $brooklyn:config(\"custom.stabilizationDelay\")",
+ " entityFailed.republishTime: $brooklyn:config(\"custom.republishTime\")");
--- End diff --
You're right. I change it to have two children, and the test failed (because it didn't merge to the top-level). I've therefore also changed the yaml to use `$brooklyn:scopeRoot().config(...)`.
---
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] brooklyn-server pull request #440: BROOKLYN-345: persist brooklyn.parameters...
Posted by neykov <gi...@git.apache.org>.
Github user neykov commented on a diff in the pull request:
https://github.com/apache/brooklyn-server/pull/440#discussion_r88286511
--- Diff: policy/src/main/java/org/apache/brooklyn/policy/ha/ServiceFailureDetector.java ---
@@ -230,6 +237,7 @@ protected void setActualState(Maybe<Lifecycle> state) {
}
lastPublished = LastPublished.FAILED;
entity.sensors().emit(HASensors.ENTITY_FAILED, new HASensors.FailureDescriptor(entity, getFailureDescription(now)));
+ requestPersist();
--- End diff --
Oh that's sneaky. Can you use config instead? (not sure if config change triggers persistence on adjuncts).
---
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] brooklyn-server pull request #440: BROOKLYN-345: persist brooklyn.parameters...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/brooklyn-server/pull/440
---
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] brooklyn-server pull request #440: BROOKLYN-345: persist brooklyn.parameters...
Posted by ahgittin <gi...@git.apache.org>.
Github user ahgittin commented on a diff in the pull request:
https://github.com/apache/brooklyn-server/pull/440#discussion_r90008704
--- Diff: camp/camp-brooklyn/src/test/java/org/apache/brooklyn/camp/brooklyn/ServiceFailureDetectorYamlTest.java ---
@@ -0,0 +1,189 @@
+/*
+ * 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 org.apache.brooklyn.camp.brooklyn;
+
+import static org.testng.Assert.assertEquals;
+import static org.testng.Assert.assertNotNull;
+
+import java.util.Map;
+
+import org.apache.brooklyn.api.entity.Entity;
+import org.apache.brooklyn.api.sensor.Enricher;
+import org.apache.brooklyn.config.ConfigKey;
+import org.apache.brooklyn.core.entity.RecordingSensorEventListener;
+import org.apache.brooklyn.core.entity.lifecycle.ServiceStateLogic.ServiceNotUpLogic;
+import org.apache.brooklyn.core.sensor.SensorEventPredicates;
+import org.apache.brooklyn.core.test.entity.TestEntity;
+import org.apache.brooklyn.policy.ha.HASensors;
+import org.apache.brooklyn.policy.ha.ServiceFailureDetector;
+import org.apache.brooklyn.util.time.Duration;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+import org.testng.annotations.Test;
+
+import com.google.common.base.Joiner;
+import com.google.common.base.Predicates;
+import com.google.common.collect.ImmutableMap;
+import com.google.common.collect.Iterables;
+
+@Test
+public class ServiceFailureDetectorYamlTest extends AbstractYamlTest {
+
+ @SuppressWarnings("unused")
+ private static final Logger log = LoggerFactory.getLogger(ServiceFailureDetectorYamlTest.class);
+
+ static final String INDICATOR_KEY_1 = "test-indicator-1";
+
+ static final String appId = "my-app";
+ static final String appVersion = "1.2.3";
+ static final String appVersionedId = appId + ":" + appVersion;
+
+ static final String catalogYamlSimple = Joiner.on("\n").join(
+ "brooklyn.catalog:",
+ " id: " + appId,
+ " version: " + appVersion,
+ " itemType: entity",
+ " item:",
+ " type: " + TestEntity.class.getName(),
+ " brooklyn.enrichers:",
+ " - type: " + ServiceFailureDetector.class.getName());
+
+ static final String catalogYamlWithDsl = Joiner.on("\n").join(
+ "brooklyn.catalog:",
+ " id: my-app",
+ " version: 1.2.3",
+ " itemType: entity",
+ " item:",
+ " services:",
+ " - type: " + TestEntity.class.getName(),//FailingEntity.class.getName(),
+ " brooklyn.parameters:",
+ " - name: custom.stabilizationDelay",
+ " type: " + Duration.class.getName(),
+ " default: 1ms",
+ " - name: custom.republishTime",
+ " type: " + Duration.class.getName(),
+ " default: 1m",
+ " brooklyn.enrichers:",
+ " - type: " + ServiceFailureDetector.class.getName(),
+ " brooklyn.config:",
+ " serviceOnFire.stabilizationDelay: $brooklyn:config(\"custom.stabilizationDelay\")",
+ " entityFailed.stabilizationDelay: $brooklyn:config(\"custom.stabilizationDelay\")",
+ " entityRecovered.stabilizationDelay: $brooklyn:config(\"custom.stabilizationDelay\")",
+ " entityFailed.republishTime: $brooklyn:config(\"custom.republishTime\")");
+
+ static final String catalogYamlWithDslReferenceParentDefault = Joiner.on("\n").join(
+ "brooklyn.catalog:",
+ " id: my-app",
+ " version: 1.2.3",
+ " itemType: entity",
+ " item:",
+ " brooklyn.parameters:",
+ " - name: custom.stabilizationDelay",
+ " type: " + Duration.class.getName(),
+ " default: 1ms",
+ " - name: custom.republishTime",
+ " type: " + Duration.class.getName(),
+ " default: 1m",
+ " services:",
+ " - type: " + TestEntity.class.getName(),//FailingEntity.class.getName(),
+ " brooklyn.enrichers:",
+ " - type: " + ServiceFailureDetector.class.getName(),
+ " brooklyn.config:",
+ " serviceOnFire.stabilizationDelay: $brooklyn:config(\"custom.stabilizationDelay\")",
+ " entityFailed.stabilizationDelay: $brooklyn:config(\"custom.stabilizationDelay\")",
+ " entityRecovered.stabilizationDelay: $brooklyn:config(\"custom.stabilizationDelay\")",
+ " entityFailed.republishTime: $brooklyn:config(\"custom.republishTime\")");
--- End diff --
With #462 this might be able to be improved.
---
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] brooklyn-server issue #440: BROOKLYN-345: persist brooklyn.parameters (fixes...
Posted by ahgittin <gi...@git.apache.org>.
Github user ahgittin commented on the issue:
https://github.com/apache/brooklyn-server/pull/440
looks good, tests passing. merging. some follow-up to do in #462 which i'll do next.
---
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] brooklyn-server pull request #440: BROOKLYN-345: persist brooklyn.parameters...
Posted by neykov <gi...@git.apache.org>.
Github user neykov commented on a diff in the pull request:
https://github.com/apache/brooklyn-server/pull/440#discussion_r90013209
--- Diff: core/src/main/java/org/apache/brooklyn/core/config/internal/AbstractConfigMapImpl.java ---
@@ -163,13 +163,18 @@ public ConfigBag getLocalConfigBag() {
return putAllOwnConfigIntoSafely(ConfigBag.newInstance()).seal();
}
- public Object setConfig(ConfigKey<?> key, Object v) {
- Object val = coerceConfigVal(key, v);
+ public Object setConfig(final ConfigKey<?> key, Object v) {
+ // Use our own key for writing, (e.g. in-case it should (or should not) be a structured key like MapConfigKey).
+ // This is same logic as for getConfig, except we only have to look at our own container.
+ ConfigKey<?> ownKey = getKeyAtContainer(getContainer(), key);
+ if (ownKey==null) ownKey = key;
+
+ Object val = coerceConfigVal(ownKey, v);
--- End diff --
I guess it's useful for catching errors early. On the other hand the uncertainty of what config key (and type) will be used to get values vs setting them makes it harder to reason about the behaviour.
Say we are setting a double with an integer key - it will throw away the fractional part. Then getting it back with a `Double` key.
---
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] brooklyn-server pull request #440: BROOKLYN-345: persist brooklyn.parameters...
Posted by neykov <gi...@git.apache.org>.
Github user neykov commented on a diff in the pull request:
https://github.com/apache/brooklyn-server/pull/440#discussion_r88283163
--- Diff: core/src/main/java/org/apache/brooklyn/core/mgmt/rebind/dto/BasicEntityMemento.java ---
@@ -139,17 +142,28 @@ protected BasicEntityMemento(Builder builder) {
effectors = toPersistedList(builder.effectors);
+ allConfigKeys = builder.configKeys;
configByKey = builder.config;
configUnmatched = builder.configUnmatched;
attributesByKey = builder.attributes;
+ staticConfigKeys = getStaticConfigKeys();
+ staticSensorKeys = getStaticSensorKeys();
+
if (configByKey!=null) {
configKeys = Maps.newLinkedHashMap();
config = Maps.newLinkedHashMap();
+
+ for (ConfigKey<?> key : allConfigKeys) {
+ if (!key.equals(staticConfigKeys.get(key.getName()))) {
+ configKeys.put(key.getName(), key);
+ }
+ }
--- End diff --
If there's no `configByKey` you might still want to initialize `configKeys`.
---
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] brooklyn-server pull request #440: BROOKLYN-345: persist brooklyn.parameters...
Posted by neykov <gi...@git.apache.org>.
Github user neykov commented on a diff in the pull request:
https://github.com/apache/brooklyn-server/pull/440#discussion_r88287453
--- Diff: utils/common/src/main/java/org/apache/brooklyn/util/time/Time.java ---
@@ -157,7 +157,7 @@ public static String makeTimeStringRounded(long t, TimeUnit unit) {
return makeTimeStringNanoRounded(nanos);
}
public static String makeTimeStringRounded(Stopwatch timer) {
- return makeTimeStringRounded(timer.elapsed(TimeUnit.MILLISECONDS), TimeUnit.MILLISECONDS);
+ return (timer == null) ? null : makeTimeStringRounded(timer.elapsed(TimeUnit.MILLISECONDS), TimeUnit.MILLISECONDS);
--- End diff --
Can you add `@Nullable`, useful when looking at the signature from the call site.
---
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] brooklyn-server pull request #440: BROOKLYN-345: persist brooklyn.parameters...
Posted by neykov <gi...@git.apache.org>.
Github user neykov commented on a diff in the pull request:
https://github.com/apache/brooklyn-server/pull/440#discussion_r89965074
--- Diff: core/src/main/java/org/apache/brooklyn/core/config/internal/AbstractConfigMapImpl.java ---
@@ -163,13 +163,18 @@ public ConfigBag getLocalConfigBag() {
return putAllOwnConfigIntoSafely(ConfigBag.newInstance()).seal();
}
- public Object setConfig(ConfigKey<?> key, Object v) {
- Object val = coerceConfigVal(key, v);
+ public Object setConfig(final ConfigKey<?> key, Object v) {
+ // Use our own key for writing, (e.g. in-case it should (or should not) be a structured key like MapConfigKey).
+ // This is same logic as for getConfig, except we only have to look at our own container.
+ ConfigKey<?> ownKey = getKeyAtContainer(getContainer(), key);
+ if (ownKey==null) ownKey = key;
+
+ Object val = coerceConfigVal(ownKey, v);
--- End diff --
I think we should keep the "config coerces on get, sensors on put" rule. So remove this coerce.
---
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] brooklyn-server pull request #440: BROOKLYN-345: persist brooklyn.parameters...
Posted by neykov <gi...@git.apache.org>.
Github user neykov commented on a diff in the pull request:
https://github.com/apache/brooklyn-server/pull/440#discussion_r88286904
--- Diff: policy/src/main/java/org/apache/brooklyn/policy/ha/ServiceFailureDetector.java ---
@@ -230,6 +237,7 @@ protected void setActualState(Maybe<Lifecycle> state) {
}
lastPublished = LastPublished.FAILED;
entity.sensors().emit(HASensors.ENTITY_FAILED, new HASensors.FailureDescriptor(entity, getFailureDescription(now)));
+ requestPersist();
--- End diff --
Yes it does.
---
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.
---