Github user ahgittin commented on a diff in the pull request:
https://github.com/apache/brooklyn-server/pull/868#discussion_r147361992
--- 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 --
that would be nice. yes, pseudo-osgi `OsgiManager` might be cleaner than
checks throughout our code, depending how much smarts are needed there.
alternatively it could spin up felix on demand.
other thing to consider is _only_ osgi, though that make make running
individual IDE tests too cumbersome to set up and/or slow to run.
---
