brooklyn-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "ASF GitHub Bot (JIRA)" <j...@apache.org>
Subject [jira] [Commented] (BROOKLYN-345) NPE on rebind of enricher ServiceStateLogic$ComputeServiceState
Date Fri, 02 Dec 2016 09:08:58 GMT

    [ https://issues.apache.org/jira/browse/BROOKLYN-345?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=15714540#comment-15714540
] 

ASF GitHub Bot commented on BROOKLYN-345:
-----------------------------------------

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


> NPE on rebind of enricher ServiceStateLogic$ComputeServiceState
> ---------------------------------------------------------------
>
>                 Key: BROOKLYN-345
>                 URL: https://issues.apache.org/jira/browse/BROOKLYN-345
>             Project: Brooklyn
>          Issue Type: Bug
>            Reporter: Aled Sage
>
> Using Brooklyn 0.10.0-SNAPSHOT (e.g. brooklyn-server commit 66b9b1c)...
> When rebinding to existing persisted state, it failed to create the enricher {{ServiceStateLogic$ComputeServiceState}}
with the NPE shown below (but then continued, so this is otherwise benign):
> {noformat}
> 2016-09-09 09:31:03,850 WARN  o.a.b.c.m.r.RebindExceptionHandlerImpl [brooklyn-execmanager-VtZheMDn-0]:
problem adding enricher cdoy70m1hv (ServiceFailureDetector{name=org.apache.brooklyn.policy.ha.ServiceFailureDetector,
uniqueTag=service.state.actual, running=true, entity=VanillaSoftwareProcessImpl{id=pwz8z4pbyp},
id=cdoy70m1hv}) to entity pwz8z4pbyp (VanillaSoftwareProcessImpl{id=pwz8z4pbyp}); continuing
> java.lang.NullPointerException: null
>         at org.apache.brooklyn.policy.ha.ServiceFailureDetector.setActualState(ServiceFailureDetector.java:188)
~[brooklyn-policy-0.10.0-20160907.0931.jar:0.10.0-20160907.0931]
>         at org.apache.brooklyn.core.entity.lifecycle.ServiceStateLogic$ComputeServiceState.onEvent(ServiceStateLogic.java:288)
~[brooklyn-core-0.10.0-20160907.0931.jar:0.10.0-20160907.0931]
>         at org.apache.brooklyn.policy.ha.ServiceFailureDetector.onEvent(ServiceFailureDetector.java:159)
~[brooklyn-policy-0.10.0-20160907.0931.jar:0.10.0-20160907.0931]
>         at org.apache.brooklyn.core.entity.lifecycle.ServiceStateLogic$ComputeServiceState.setEntity(ServiceStateLogic.java:274)
~[brooklyn-core-0.10.0-20160907.0931.jar:0.10.0-20160907.0931]
>         at org.apache.brooklyn.core.entity.AbstractEntity$BasicEnricherSupport.add(AbstractEntity.java:1788)
~[brooklyn-core-0.10.0-20160907.0931.jar:0.10.0-20160907.0931]
>         at org.apache.brooklyn.core.mgmt.rebind.BasicEntityRebindSupport.addEnrichers(BasicEntityRebindSupport.java:145)
[brooklyn-core-0.10.0-20160907.0931.jar:0.10.0-20160907.0931]
>         at org.apache.brooklyn.core.mgmt.rebind.BasicEntityRebindSupport.addEnrichers(BasicEntityRebindSupport.java:47)
[brooklyn-core-0.10.0-20160907.0931.jar:0.10.0-20160907.0931]
>         at org.apache.brooklyn.core.mgmt.rebind.RebindIteration.associateAdjunctsWithEntities(RebindIteration.java:650)
[brooklyn-core-0.10.0-20160907.0931.jar:0.10.0-20160907.0931]
>         at org.apache.brooklyn.core.mgmt.rebind.RebindIteration.doRun(RebindIteration.java:244)
[brooklyn-core-0.10.0-20160907.0931.jar:0.10.0-20160907.0931]
>         at org.apache.brooklyn.core.mgmt.rebind.InitialFullRebindIteration.doRun(InitialFullRebindIteration.java:69)
[brooklyn-core-0.10.0-20160907.0931.jar:0.10.0-20160907.0931]
>         at org.apache.brooklyn.core.mgmt.rebind.RebindIteration.run(RebindIteration.java:266)
[brooklyn-core-0.10.0-20160907.0931.jar:0.10.0-20160907.0931]
>         at org.apache.brooklyn.core.mgmt.rebind.RebindManagerImpl.rebindImpl(RebindManagerImpl.java:558)
[brooklyn-core-0.10.0-20160907.0931.jar:0.10.0-20160907.0931]
>         at org.apache.brooklyn.core.mgmt.rebind.RebindManagerImpl$3.call(RebindManagerImpl.java:508)
[brooklyn-core-0.10.0-20160907.0931.jar:0.10.0-20160907.0931]
>         at org.apache.brooklyn.core.mgmt.rebind.RebindManagerImpl$3.call(RebindManagerImpl.java:506)
[brooklyn-core-0.10.0-20160907.0931.jar:0.10.0-20160907.0931]
>         at org.apache.brooklyn.util.core.task.BasicExecutionManager$SubmissionCallable.call(BasicExecutionManager.java:519)
[brooklyn-core-0.10.0-20160907.0931.jar:0.10.0-20160907.0931]
>         at java.util.concurrent.FutureTask.run(FutureTask.java:262) [na:1.7.0_95]
>         at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1145)
[na:1.7.0_95]
>         at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:615)
[na:1.7.0_95]
>         at java.lang.Thread.run(Thread.java:745) [na:1.7.0_95]
> {noformat}
> The line throwing the NPE is executing {{setEntityOnFireTime = now + getConfig(SERVICE_ON_FIRE_STABILIZATION_DELAY).toMilliseconds()}}.
That suggests we got null for the config value, even though there is a default value of zero
for it.
> The enricher persisted state contains:
> {noformat}
>     <serviceOnFire.stabilizationDelay>
>       <org.apache.brooklyn.camp.brooklyn.spi.dsl.methods.DslComponent_-DslConfigSupplier>
>         <component>
>           <componentId></componentId>
>           <scope>THIS</scope>
>         </component>
>         <keyName>swarm.recovery.stabilizationDelay</keyName>
>       </org.apache.brooklyn.camp.brooklyn.spi.dsl.methods.DslComponent_-DslConfigSupplier>
>     </serviceOnFire.stabilizationDelay>
> {noformat}
> And the owning entity has:
> {noformat}
>   brooklyn.parameters:
>     - name: swarm.recovery.stabilizationDelay
>       label: Stabilization Delay
>       description: |
>         Time period for which the service must be consistently in the same state to trigger
an action
>       # A restart shouldn&apos;t trigger failure
>       type: org.apache.brooklyn.util.time.Duration
>       default: 5m
> {noformat}
> There are a few things we should think about fixing:
> 1. guard against the NPE in {{ServiceFailureDetector.setActualState}} (i.e. handle when
the config returns null) - but unclear what value it should then default to.
> 2. avoid calling this code on rebind. for example, instead of {{ComputeServiceState.setEntity}}
immediately calling {{onEvent(null)}}, it could subscribe with "notifyOfInitialValue" so that
it gets a callback (in the right thread, at the right time).
> 3. investigate further why the config lookup returned null - e.g. is it because entity
wasn't fully initialised, or because the DSL didn't find the default value defined in brooklyn.parameters?



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)

Mime
View raw message