You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@brooklyn.apache.org by aledsage <gi...@git.apache.org> on 2017/12/16 09:59:59 UTC
[GitHub] brooklyn-server pull request #917: BROOKLYN-570: fix entity synchronizing on...
GitHub user aledsage opened a pull request:
https://github.com/apache/brooklyn-server/pull/917
BROOKLYN-570: fix entity synchronizing on self
Fixes https://issues.apache.org/jira/browse/BROOKLYN-570
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/aledsage/brooklyn-server BROOKLYN-570-fix-synchronizedOnEntity
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/brooklyn-server/pull/917.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 #917
----
commit 704ad5e8ad729f5a35c29d7eb29009cd7fb09bc4
Author: Aled Sage <al...@gmail.com>
Date: 2017-12-16T09:58:13Z
BROOKLYN-570: fix entity synchronizing on self
----
---
[GitHub] brooklyn-server pull request #917: BROOKLYN-570: fix entity synchronizing on...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/brooklyn-server/pull/917
---
[GitHub] brooklyn-server issue #917: BROOKLYN-570: fix entity synchronizing on self
Posted by tbouron <gi...@git.apache.org>.
Github user tbouron commented on the issue:
https://github.com/apache/brooklyn-server/pull/917
Thanks @aledsage. Tests passed, merging this now
---
[GitHub] brooklyn-server pull request #917: BROOKLYN-570: fix entity synchronizing on...
Posted by aledsage <gi...@git.apache.org>.
Github user aledsage commented on a diff in the pull request:
https://github.com/apache/brooklyn-server/pull/917#discussion_r158293220
--- Diff: core/src/main/java/org/apache/brooklyn/core/entity/AbstractEntity.java ---
@@ -749,13 +756,15 @@ public Application getApplication() {
// FIXME Can this really be deleted? Overridden by AbstractApplication; needs careful review
/** @deprecated since 0.4.0 should not be needed / leaked outwith brooklyn internals / mgmt support? */
@Deprecated
- protected synchronized void setApplication(Application app) {
- if (application != null) {
- if (application.getId() != app.getId()) {
- throw new IllegalStateException("Cannot change application of entity (attempted for "+this+" from "+getApplication()+" to "+app);
+ protected void setApplication(Application app) {
+ synchronized (appMutex) {
--- End diff --
We don't ever want an entity-author to call `setApplication`, so I think I'm ok leaving it deprecated. Not sure what we should do long term to hide this away somewhere in framework code so that users don't accidentally call it or get confused by it.
The use of `FIXME` is too strong; I'll change it to a `TODO` - it's low priority as it does work, it's just ugly.
---
For where to put the synchronized, I think it needs to be here.
---
[GitHub] brooklyn-server pull request #917: BROOKLYN-570: fix entity synchronizing on...
Posted by tbouron <gi...@git.apache.org>.
Github user tbouron commented on a diff in the pull request:
https://github.com/apache/brooklyn-server/pull/917#discussion_r158235800
--- Diff: core/src/main/java/org/apache/brooklyn/core/entity/AbstractEntity.java ---
@@ -749,13 +756,15 @@ public Application getApplication() {
// FIXME Can this really be deleted? Overridden by AbstractApplication; needs careful review
/** @deprecated since 0.4.0 should not be needed / leaked outwith brooklyn internals / mgmt support? */
@Deprecated
- protected synchronized void setApplication(Application app) {
- if (application != null) {
- if (application.getId() != app.getId()) {
- throw new IllegalStateException("Cannot change application of entity (attempted for "+this+" from "+getApplication()+" to "+app);
+ protected void setApplication(Application app) {
+ synchronized (appMutex) {
--- End diff --
@aledsage Is it the right place to synchronize as this method is `deprecated`? It looks like the answer is yes but just wanted to check.
Do you think the `@Deprecated` annotation can be removed? Or at least the `FIXME` comment?
---