brooklyn-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From aledsage <...@git.apache.org>
Subject [GitHub] brooklyn-server pull request #868: allow types from different bundles if equ...
Date Thu, 26 Oct 2017 10:18:35 GMT
Github user aledsage commented on a diff in the pull request:

    https://github.com/apache/brooklyn-server/pull/868#discussion_r147098752
  
    --- Diff: core/src/main/java/org/apache/brooklyn/core/typereg/BasicBrooklynTypeRegistry.java
---
    @@ -368,13 +399,77 @@ public void addToLocalUnpersistedTypeRegistry(RegisteredType type,
boolean canFo
             if (!type.getId().equals(type.getSymbolicName()+":"+type.getVersion()))
                 Asserts.fail("Registered type "+type+" has ID / symname mismatch");
             
    -        RegisteredType oldType = mgmt.getTypeRegistry().get(type.getId());
    -        if (oldType==null || canForce || BrooklynVersionSyntax.isSnapshot(oldType.getVersion()))
{
    -            log.debug("Inserting "+type+" into "+this);
    -            localRegisteredTypes.put(type.getId(), type);
    -        } else {
    -            assertSameEnoughToAllowReplacing(oldType, type);
    -        }
    +        Locks.withLock(localRegistryLock.writeLock(),
    +            () -> {
    +                Map<String, RegisteredType> knownMatchingTypesByBundles = localRegisteredTypesAndContainingBundles.get(type.getId());
    +                if (knownMatchingTypesByBundles==null) {
    +                    knownMatchingTypesByBundles = MutableMap.of();
    +                    localRegisteredTypesAndContainingBundles.put(type.getId(), knownMatchingTypesByBundles);
    +                }
    +
    +                Set<String> oldContainingBundlesToRemove = MutableSet.of();
    +                boolean newIsWrapperBundle = isWrapperBundle(type.getContainingBundle());
    +                for (RegisteredType existingT: knownMatchingTypesByBundles.values())
{
    +                    String reasonForDetailedCheck = null;
    +                    boolean sameBundle = Objects.equals(existingT.getContainingBundle(),
type.getContainingBundle());
    +                    boolean oldIsWrapperBundle = isWrapperBundle(existingT.getContainingBundle());
    +                    if (sameBundle || (oldIsWrapperBundle && newIsWrapperBundle))
{
    +                        // allow replacement (different plan for same type) if either
    +                        // it's the same bundle or the old one was a wrapper, AND
    +                        // either we're forced or in snapshot-land
    +                        if (!sameBundle) {
    +                            // if old is wrapper bundle, we have to to remove the old
record
    +                            oldContainingBundlesToRemove.add(existingT.getContainingBundle());
    +                        }
    +                        if (canForce) {
    +                            log.debug("Addition of "+type+" to replace "+existingT+"
allowed because force is on");
    +                            continue;
    +                        }
    +                        if (BrooklynVersionSyntax.isSnapshot(type.getVersion())) {
    +                            if (existingT.getContainingBundle()!=null) {
    +                                if (BrooklynVersionSyntax.isSnapshot(VersionedName.fromString(existingT.getContainingBundle()).getVersionString()))
{
    +                                    log.debug("Addition of "+type+" to replace "+existingT+"
allowed because both are snapshot");
    +                                    continue;
    +                                } else {
    +                                    reasonForDetailedCheck = "the containing bundle "+existingT.getContainingBundle()+"
is not a SNAPSHOT and addition is not forced";
    +                                }
    +                            } else {
    +                                // can this occur?
    +                                reasonForDetailedCheck = "the containing bundle of the
type is unknown (cannot confirm it is snapshot)";
    +                            }
    +                        } else {
    +                            reasonForDetailedCheck = "the type is not a SNAPSHOT and
addition is not forced";
    +                        }
    +                    } else if (oldIsWrapperBundle) {
    +                        reasonForDetailedCheck = type.getId()+" is in a named bundle
replacing an item from an anonymous bundle-wrapped BOM, so definitions must be the same (or
else give it a different version)";
    +                    } else if (newIsWrapperBundle) {
    +                        reasonForDetailedCheck = type.getId()+" is in an anonymous bundle-wrapped
BOM replacing an item from a named bundle, so definitions must be the same (or else give it
a different version)";
    +                    } else {
    +                        reasonForDetailedCheck = type.getId()+" is defined in different
bundle";
    +                    }
    +                    assertSameEnoughToAllowReplacing(existingT, type, reasonForDetailedCheck);
    +                }
    +            
    +                log.debug("Inserting "+type+" into "+this+
    +                    (oldContainingBundlesToRemove.isEmpty() ? "" : " (removing entry
from "+oldContainingBundlesToRemove+")"));
    +                for (String oldContainingBundle: oldContainingBundlesToRemove) {
    +                    knownMatchingTypesByBundles.remove(oldContainingBundle);
    +                }
    +                knownMatchingTypesByBundles.put(type.getContainingBundle(), type);
    +            });
    +    }
    +
    +    private boolean isWrapperBundle(String bundleNameVersion) { 
    +        if (bundleNameVersion==null) return true;
    +        Maybe<OsgiManager> osgi = ((ManagementContextInternal)mgmt).getOsgiManager();
    +        // if not osgi, everything is treated as a wrapper bundle
    +        if (osgi.isAbsent()) return true;
    --- End diff --
    
    [bigger topic than this PR, but raising it here while I'm thinking about it].
    
    Now that osgi is the "only" way to run brooklyn (except for tests, and for developers
using a quick launch in their IDE?), I wonder how we want to handle `osgi.isAbsent()` long
term. For example, instead of handling it in our code in lots of places, perhaps tests should
inject a pseudo-osgi-manager that just tracks bundle install/delete/find, and does a simple
peek inside it for a catalog.bom at the root. Anything more complicated would fail-fast.
    
    Let's discuss that separately, but it does feel like a way to simplify the production
code (making our test framework code slightly more complicated instead).


---

Mime
View raw message