[GitHub] brooklyn-server issue #462: Inherit config default values
Github user ahgittin commented on the issue: https://github.com/apache/brooklyn-server/pull/462 Seems there may be a pathway by which REST does not populate an entity's `EntityType.configKeys` in this PR, reported by @sjcorbett . Looking in to this now. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] brooklyn-server issue #462: Inherit config default values
Github user ahgittin commented on the issue: https://github.com/apache/brooklyn-server/pull/462 Good spot. Will fix and merge. --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] brooklyn-server issue #462: Inherit config default values
Github user ahgittin commented on the issue: https://github.com/apache/brooklyn-server/pull/462 Could cause an issue if absents are used as keys with the intention that they all be different but that would be weird. Have checked our hashCode and it's a constant for absents 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] brooklyn-server issue #462: Inherit config default values
Github user ahgittin commented on the issue: https://github.com/apache/brooklyn-server/pull/462 @neykov @geomacy @aledsage Done. Hurrah. I've fixed the "declared keys" exclusions -- in the end @geomacy's suggestion re cleaning up the `resolveParameters` method was on point, and the missing case @neykov asked about was guaranteed not to occur, so there are a few other cleanups also. Apologies for the complexity of this all (I wish we could move to YOML and do away with `Entity` subclasses!). The main new PRs to review are the last two. The others are mostly dull fixes. --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] brooklyn-server issue #462: Inherit config default values
Github user ahgittin commented on the issue: https://github.com/apache/brooklyn-server/pull/462 ^^ and neykov's comments re `SpecParameterForInheritance` --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] brooklyn-server issue #462: Inherit config default values
Github user ahgittin commented on the issue: https://github.com/apache/brooklyn-server/pull/462 more updates; now just need to: * fix the bug geomacy found * rewview comments from neykov re SpecParameterUnwrappingTest.java --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] brooklyn-server issue #462: Inherit config default values
Github user ahgittin commented on the issue: https://github.com/apache/brooklyn-server/pull/462 @geomacy good find in your failing test [above](https://github.com/apache/brooklyn-server/pull/462#issuecomment-270408039) -- we do not filter out hidden ancestor config keys whenever we populate the list of declared config keys, neither in: * `BasicSpecParameter.fromSpec(..)`, where we extend a spec from a spec; nor * `InternalEntityFactory.loadUnitializedEntity(...)`, where we create a spec for a java class; nor * `BrooklynDynamicType.buildConfigKeys(..)`, where we record the entity signature for a java type i think it is worth fixing 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] brooklyn-server issue #462: Inherit config default values
Github user ahgittin commented on the issue: https://github.com/apache/brooklyn-server/pull/462 failure appears non-deterministic and i think unrelated ``` Tests run: 2036, Failures: 1, Errors: 0, Skipped: 0, Time elapsed: 328.296 sec <<< FAILURE! - in TestSuite (org.apache.brooklyn.core.entity.ApplicationLifecycleStateTest) Time elapsed: 0.183 sec <<< FAILURE! java.lang.AssertionError: entity=Application[1naq8bil]; attribute=Sensor: service.state (org.apache.brooklyn.core.entity.lifecycle.Lifecycle) expected [running] but found [on-fire] at org.apache.brooklyn.core.entity.ApplicationLifecycleStateTest.assertHealthContinually(ApplicationLifecycleStateTest.java:196) at org.apache.brooklyn.core.entity.ApplicationLifecycleStateTest.testStartsThenChildFailsButWithQuorumCausesAppToSucceed(ApplicationLifecycleStateTest.java:170) ``` and ``` Failed tests: org.apache.brooklyn.core.entity.ApplicationLifecycleStateTest.(org.apache.brooklyn.core.entity.ApplicationLifecycleStateTest) Run 1: PASS Run 2: PASS Run 3: PASS Run 4: PASS Run 5: PASS Run 6: PASS Run 7: PASS Run 8: ApplicationLifecycleStateTest.testStartsThenChildFailsButWithQuorumCausesAppToSucceed:170->assertHealthContinually:196 entity=Application[1naq8bil]; attribute=Sensor: service.state (org.apache.brooklyn.core.entity.lifecycle.Lifecycle) expected [running] but found [on-fire] Run 9: PASS Run 10: PASS ``` --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] brooklyn-server issue #462: Inherit config default values
Github user ahgittin commented on the issue: https://github.com/apache/brooklyn-server/pull/462 Thanks for such great comments. Nearly done, should finish today. Sorry I dropped the ball on 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] brooklyn-server issue #462: Inherit config default values
Github user geomacy commented on the issue: https://github.com/apache/brooklyn-server/pull/462 In general I think this looks good to me and does what it says on the tin; I just had the few observations and questions above. --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] brooklyn-server issue #462: Inherit config default values
Github user geomacy commented on the issue:
https://github.com/apache/brooklyn-server/pull/462
Wrote a few extra tests to try out the change. The only one that didn't
perform as I'd expected was
```java
@Test
public void
testConfigDefaultIsNotInheritedWith_LocalDefaultResolvesWithAncestorValue_SetToTrue()
throws Exception {
addCatalogItems(
"brooklyn.catalog:",
" itemType: entity",
" items:",
" - id: entity-with-keys",
"item:",
" type: "+TestEntity.class.getName(),
" brooklyn.parameters:",
" - name: my.param.key",
"type: string",
"inheritance.parent: never",
"description: description one",
"default: myDefaultVal",
" brooklyn.config:",
"my.other.key: $brooklyn:config(\"my.param.key\")");
addCatalogItems(
"brooklyn.catalog:",
" itemType: entity",
" items:",
" - id: wrapper-entity",
"item:",
" brooklyn.parameters:",
" - name: my.param.key",
"description: description two",
" type: entity-with-keys");
String yaml = Joiner.on("\n").join(
"services:",
"- type: wrapper-entity");
Entity app = createStartWaitAndLogApplication(yaml);
final TestEntity entity = (TestEntity)
Iterables.getOnlyElement(app.getChildren());
LOG.info("Config keys declared on "+entity+":
"+entity.config().findKeysDeclared(Predicates.alwaysTrue()));
ConfigKey key = Iterables.getOnlyElement(
entity.config().findKeysDeclared(ConfigPredicates.nameEqualTo("my.param.key"))
);
assertEquals(key.getDescription(), "description two");
assertEquals(entity.config().get(key), null);
}
```
noting that the last assertion above expects the value to be null because
the "inheritance.parent"
has been set to "never". Actually the failure is
```
java.lang.AssertionError: expected [null] but found [myDefaultVal]
Expected :null
Actual :myDefaultVal
```
I debugged to see what goes on here, and found that the
BasicConfigInheritance class is
doing what I expected, and the default value is *not* inherited. Instead,
the default
is set higher up in the stack, in
AbstractConfigurationSupportInternal#getNonBlockingResolvingSimple,
at the
[line](https://github.com/apache/brooklyn-server/blob/master/core/src/main/java/org/apache/brooklyn/core/objs/AbstractConfigurationSupportInternal.java#L139)
```
Object unresolved = getRaw(key).or(key.getDefaultValue());
```
so in this case the higher level code is ignoring and overriding the
instructions of the
way the inheritance is configured on the key. Maybe now that the
inheritance code is
written this line and any others like it should be changed, to avoid
"second-guessing"
the specified inheritance behaviour.
---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] brooklyn-server issue #462: Inherit config default values
Github user ahgittin commented on the issue: https://github.com/apache/brooklyn-server/pull/462 rebased on master; still need to address comments above --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
