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

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

Github user ahgittin commented on a diff in the pull request:

    https://github.com/apache/brooklyn-server/pull/338#discussion_r112694452
  
    --- Diff: 
api/src/main/java/org/apache/brooklyn/api/internal/AbstractBrooklynObjectSpec.java
 ---
    @@ -91,26 +96,39 @@ public String toString() {
         }
     
         protected abstract void checkValidType(Class<? extends T> type);
    -    
    +
         public SpecT displayName(String val) {
             displayName = val;
             return self();
         }
    -    
    -    /** Set the catalog item ID that defined this object, also used for 
searching for type and resources referenced */
    -    // since https://issues.apache.org/jira/browse/BROOKLYN-445 this must 
no longer be used to indicate
    -    // a caller-context catalog item that should be used for search 
purposes;
    -    // if that behaviour is desired, the child should be refactored to be 
its own item in the catalog BOM
    -    // (or TODO we add a separate field to record other catalog item IDs 
that could be applied for searching, see below)
    +
    +    /**
    +     * Set the catalog item ID that defined this object;
    +     * since https://issues.apache.org/jira/browse/BROOKLYN-445 this must 
no longer be used to indicate
    +     * a caller-context catalog item that should be used for search 
purposes;
    +     * instead use {@link #catalogItemIdAndSearchPath}.
    +     */
         public SpecT catalogItemId(String val) {
             catalogItemId = val;
             return self();
         }
    -    // TODO in many places (callers to this method) we prefer a wrapper 
item ID;
    -    // that is right, because the wrapper's defn will refer to the wrapped,
    -    // but we might need also to collect the item ID's so that *all* can 
be searched, see #catalogItemId.
    -    // e.g. if R3 references R2 which references R1 any one of these might 
supply config keys 
    -    // referencing resources or types in their local bundles. 
    +
    +    /**
    +     * Set the immediate catalog item ID of this object, and the search 
path of other catalog items used to define it.
    +     */
    +    public synchronized SpecT catalogItemIdAndSearchPath(String 
catalogItemId, Collection<String> searchPath) {
    +        if (catalogItemId != null) {
    --- End diff --
    
    what is the reason for this behaviour on null?  ignoring searchPath seems 
weird.


> Search path and meaning of catalogItemId is overloaded
> ------------------------------------------------------
>
>                 Key: BROOKLYN-445
>                 URL: https://issues.apache.org/jira/browse/BROOKLYN-445
>             Project: Brooklyn
>          Issue Type: Bug
>    Affects Versions: 0.10.0
>            Reporter: Alex Heneveld
>
> We use catalogItemId on a spec or BrooklynObject for two things:
> (1) Record the catalog item that was defined to create an item
> (2) Find the search path to use when looking up resources and/or creating 
> other specs
> Most of the time these two are the same, e.g. an entity comes from catalog 
> item `cassandra-node:1.0` and the implementation should use the bundles 
> defined against that to resolve scripts etc.  The catalogItemId is the only 
> record of the catalog-entry that was used to define the entity; the entity 
> spec reduces it to the java type which might tell you one bundle but won't 
> tell you of other library entries from the definition.
> However in some cases we seem to want the context catalogItemId to be 
> inherited, e.g. we reference a stock type like `VanillaServer` as a child but 
> supply config pointing at scripts/images in the parent's bundle.  This is 
> currently achieved by a line in AbstractBrooklynObjectSpec's constructor 
> which sets the catalogItemId from the thread context entity's catalog item 
> ID.  However there are a couple problems with this:
> (A) We can't tell if the `catalogItemId` is the definition of an entity (it 
> usually is, but sometimes might be inherited), so we can't tell what was used 
> to create an entity
> (B) A child's search behaviour differs depending whether that child comes 
> from another catalogItemId (which will overwrite the inherited context 
> catalogItemId) or a stock item (e.g. a java type is passed to the spec 
> constructor)
> (C) When setting the EntityDynamicType we can't tell whether to clear the 
> config keys set there; in InternalEntityFactory.addSpecParameters we need to 
> know whether the spec extends the Java type defining it (in which case CAMP 
> code in BasicSpecParameter.initializeSpecWithExplicitParameters has filtered 
> out those keys which are not type-definition inheritable and set the 
> spec.parameters, so the EntityDynamicType's keys should be cleared) or not 
> (in which case spec.parameters won't normally be set and EDT.configKeys 
> should not be cleared).  Currently it checks whether there is a 
> catalogItemId, and assume it is set iff the spec is extending (former case); 
> this assumption fails if catalogItemId is inherited from context.



--
This message was sent by Atlassian JIRA
(v6.3.15#6346)

Reply via email to