You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@brooklyn.apache.org by ahgittin <gi...@git.apache.org> on 2017/07/24 16:32:01 UTC
[GitHub] brooklyn-server pull request #777: Removes `FIRST` sensors on children
GitHub user ahgittin opened a pull request:
https://github.com/apache/brooklyn-server/pull/777
Removes `FIRST` sensors on children
They were buggy and could deadlock -- and I think fundamentally flawed (as comments there suggested).
@grkvlt I think you added these; can you check that the workarounds suggested in the last commit is sufficient?
There are a few other minor tidies also.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/ahgittin/brooklyn-server primary-and-tidies
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/brooklyn-server/pull/777.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #777
----
commit 4d824127a31b56a411c29a7c4f941471ea436eed
Author: Alex Heneveld <al...@cloudsoftcorp.com>
Date: 2017-07-24T15:49:58Z
allow simple constant quora to be set as a constant (no need for complex range)
commit 14565cd13e74082f8e6494deff40683d51b5978e
Author: Alex Heneveld <al...@cloudsoftcorp.com>
Date: 2017-07-24T15:50:29Z
display nicer message when required child/quorum not healthy
commit b2251f48c46cdf88d375f8a593990f2edf860011
Author: Alex Heneveld <al...@cloudsoftcorp.com>
Date: 2017-07-24T15:51:53Z
fix errors with DynamicMultiGroup
minor things:
* javadoc
* quorum checks should only look at children (the buckets); the members are observed by the children
* if there is a problem, fail more gracefully (otherwise it risks adding nodes ad infinitum)
commit e8d25ca95a43af52c0c8a97e2da1c707f9d7fa68
Author: Alex Heneveld <al...@cloudsoftcorp.com>
Date: 2017-07-24T16:01:51Z
remove FIRST_MEMBER sensor, it doesn't work, and deprecate FIRST
both are a bit flaky, but FIRST_MEMBER especially so:
it would never get cleared, and it could cause deadlock
(as parent attempts to update child's sensor while holding members lock,
meanwhile child might publish a sensor which causes parent subscriber to try to getMembers).
additionally we no longer set FIRST=(other entity) on the child, for the same reasons,
but also it causes conflict if the child is also a group (so FIRST might be the parent's FIRST
or its own FIRST).
the new "primary" model is a much better one to use instead when you need notification
of something being promoted, or an enricher if you need the parent's first injected locally
(but you should always be able to say `$brooklyn:parent().attributeWhenReady("first"))`)
----
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] brooklyn-server issue #777: Removes `FIRST` sensors on children
Posted by ahgittin <gi...@git.apache.org>.
Github user ahgittin commented on the issue:
https://github.com/apache/brooklyn-server/pull/777
@rdowner Release notes updated now in https://github.com/apache/brooklyn-docs/pull/208 . ML discussion triggered by @tbouron (TY) and you are completely correct, that should have been done in the first instance. In my defence I was exhausted after fixing the deadlock, and the (massive) problems with the removed code (including no tests as you'll note I haven't removed any tests in this PR!) should have been found during a code review when this was added. But it is a poor excuse. TY for policing this.
As for why: Consider if an entity is added to two groups. It is unclear then what `cluster.first: true` means. A blueprint relying on that in one group may suddenly start failing if you use dynamic membership to sort entities in another way. Or if you have a group of groups, the problem gets very bad as `cluster.first` would mean both the first in your parent's group and the first in your group. On top of that the deadlock with parent setting child's sensors while holding the members lock was common and bad. I think it's not good practise to update someone else's sensors in the best of times, but to do it while holding a synch lock on an object related to the child is definitely asking for trouble. And not to have _any_ tests....
---
[GitHub] brooklyn-server issue #777: Removes `FIRST` sensors on children
Posted by rdowner <gi...@git.apache.org>.
Github user rdowner commented on the issue:
https://github.com/apache/brooklyn-server/pull/777
@ahgittin this appears to go against our published deprecation policy and is known to break some blueprints.
Your PR comments are making references to previous discussions, but it's not clear where those are, and I haven't been able to find them. Example: `as comments there suggested` in the first comment; `the new "primary" model` is mentioned in your last commit but no explanation of this model; `causes important blueprints to fail` but does not say which ones. So unfortunately I don't yet understand why this breaking change is needed.
I think we should prepare a response (explanation, workaround, new best practice recommendation) to the fact that we've broken some of our user's blueprints, with a clear justification of why it had to be done. I'd like to see this discussed on the mailing list, and noted in [the release notes](https://github.com/apache/brooklyn-docs/blob/master/guide/misc/release-notes.md), and documentation too.
Thanks
---
[GitHub] brooklyn-server issue #777: Removes `FIRST` sensors on children
Posted by ahgittin <gi...@git.apache.org>.
Github user ahgittin commented on the issue:
https://github.com/apache/brooklyn-server/pull/777
The deadlock causes important blueprints to fail so I am going to commit this now.
/cc @grkvlt @aledsage if one of you could do commit-then-review I'd appreciate it!
I'll be posting a follow-up which adds policies around managing primary elements. That should provide an even nicer way to do some of the things the removed code was trying to do.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] brooklyn-server issue #777: Removes `FIRST` sensors on children
Posted by tbouron <gi...@git.apache.org>.
Github user tbouron commented on the issue:
https://github.com/apache/brooklyn-server/pull/777
@ahgittin NP, I don't contest the why, I'm sure there was a good reason (confirmed by the above explanation) I was just surprised that this wasn't flagged before for downstream project or blueprints, that's all.
Anyway, thank you for the release notes
---
[GitHub] brooklyn-server pull request #777: Removes `FIRST` sensors on children
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/brooklyn-server/pull/777
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---