Github user ahgittin commented on a diff in the pull request:
https://github.com/apache/brooklyn-server/pull/868#discussion_r147361400
--- 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 --
Reverting / rolling-back to undo side effects is no different to as before,
is it? (Except there may be fewer types which need to be restored.)
Agree with complexity creep but feels better we take that pain than give it
to users who would have to switch and change code for new mental model which
still feels a little premature as discussed on ML. Plus I think once we nail
this I expect we can leave it alone.
+1 to snapshotting type registry for rollback and other purposes, that's
one of the reasons I prefer this approach: once legacy catalog is removed it
should be much easier than with previous legacy catalog as it's just the local
map of maps defined in this class which needs to be copied, two-levels deep;
that removes a lot of pain around rollback (plus it could allow per-tenant
views of types based on which bundles they are allowed to access)
---
