Github user aledsage commented on a diff in the pull request:
https://github.com/apache/incubator-brooklyn/pull/506#discussion_r24326988
--- Diff:
core/src/main/java/brooklyn/management/internal/LocalEntityManager.java ---
@@ -354,23 +362,23 @@ private void unmanage(final Entity e,
ManagementTransitionMode mode, boolean has
if (hasBeenReplaced) {
// we are unmanaging an old instance after having replaced it
- // (called from manage(...)
+ // don't unmanage or even clear its fields, because there
might be references to it
- if
(mode==ManagementTransitionMode.REBINDING_NO_LONGER_PRIMARY) {
- // when migrating away, these all need to be called
+ if (mode.wasReadOnly()) {
+ // if coming *from* read only; nothing needed
+ } else {
+ if (!mode.wasPrimary()) {
+ log.warn("Unexpected mode "+mode+" for
unmanage-replace "+e+" (applying anyway)");
--- End diff --
I'm a bit worried about these kind of warnings around the place, in that we
might not notice them happening until it's too late (e.g. until a customer
reports a problem). Longer term, is there something we can do to more strongly
assert what modes are expected/allowed in different circumstances?
(I'm not specifically talking about this particular piece of code; more in
general so those maintaining the code know exactly what each code branch is
for. On balance, agree it's often better to warn and continue than fail
completely in production - but in tests and QA then failing fast is arguably
better! Perhaps it should be an `assert`?)
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [email protected] or file a JIRA ticket
with INFRA.
---
