Github user aledsage commented on a diff in the pull request:
https://github.com/apache/brooklyn-server/pull/868#discussion_r147096369
--- 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);
--- End diff --
This is hard to follow - it looks reasonable, but it's hard to reason about
because there are so many situations. Also the context/granularity of how this
is used feels strange - other code like
`BasicBrooklynCatalog.collectCatalogItemsFromItemMetadataBlock` is calling this
repeatedly for each thing in a bundle (. If something goes wrong, it's then up
to the caller to revert those actions. But this call side-effects what we had
recorded against for other bundles. So hard to see how we get back to the
previous state on such a rollback.
I come back to the idea of making `bundle: ` compulsory. That would
simplify this code.
Longer term, I also think we want a "working view" of the
`BrooklynTypeRegistry` that can apply a bunch of changes and then commit or
abort them. But that's a bigger topic of conversation (completely separate from
this PR!).
---
