You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@brooklyn.apache.org by grkvlt <gi...@git.apache.org> on 2014/12/15 15:05:49 UTC
[GitHub] incubator-brooklyn pull request: Return null SubscriptionContext w...
GitHub user grkvlt opened a pull request:
https://github.com/apache/incubator-brooklyn/pull/394
Return null SubscriptionContext when management stopped
Check for null conext when calling `SubscriptionTracker` methods and allow unsubscribes with null context (harmless) but error otherwise.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/grkvlt/incubator-brooklyn fix/unsubscribe-when-stopped
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/incubator-brooklyn/pull/394.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 #394
----
commit bf6a2ec59d1dd1d52a661c52cd42a4d23dca3587
Author: Andrew Kennedy <gr...@apache.org>
Date: 2014-12-15T14:04:14Z
Return null SubscriptionContext when management stopped
----
---
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] incubator-brooklyn pull request: Return null SubscriptionContext w...
Posted by grkvlt <gi...@git.apache.org>.
Github user grkvlt commented on the pull request:
https://github.com/apache/incubator-brooklyn/pull/394#issuecomment-92905527
Question for @ahgittin or @aledsage - should I implement the same pattern as other `NonDeploment`_Blah_ classes, saving the inital management context like this:
```Java
@Override
public <T> SubscriptionHandle subscribe(Entity producer, Sensor<T> sensor, SensorEventListener<? super T> listener) {
if (isInitialManagementContextReal()) {
return initialManagementContext.getSubscriptionContext(subscriber).subscribe(producer, sensor, listener);
} else throw nonDeploymentContextError();
}
```
Also, what about a `NonDeploymentSubscriptionManager` to replace the `QueueingSubscriptionManager` as well?
---
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] incubator-brooklyn pull request: Return null SubscriptionContext w...
Posted by grkvlt <gi...@git.apache.org>.
Github user grkvlt commented on the pull request:
https://github.com/apache/incubator-brooklyn/pull/394#issuecomment-166931591
Closing this, to revisit later if necessary
---
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] incubator-brooklyn pull request: Return null SubscriptionContext w...
Posted by grkvlt <gi...@git.apache.org>.
Github user grkvlt commented on the pull request:
https://github.com/apache/incubator-brooklyn/pull/394#issuecomment-155872031
@ahgittin I had forgotten about this, but the intent is still useful. I suggest not closing this yet, until I have had a look at the code again, to see if it can be updated and made mergeable.
---
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] incubator-brooklyn pull request: Return null SubscriptionContext w...
Posted by grkvlt <gi...@git.apache.org>.
Github user grkvlt closed the pull request at:
https://github.com/apache/incubator-brooklyn/pull/394
---
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] incubator-brooklyn pull request: Return null SubscriptionContext w...
Posted by ahgittin <gi...@git.apache.org>.
Github user ahgittin commented on the pull request:
https://github.com/apache/incubator-brooklyn/pull/394#issuecomment-72014029
+1 @aledsage -- `NonDeploymentSubscriptionContext` would be simple to write and would mean most of the null checks introduced here aren't needed. follow the pattern of the other `NonDeployment*` classes.
---
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] incubator-brooklyn pull request: Return null SubscriptionContext w...
Posted by ahgittin <gi...@git.apache.org>.
Github user ahgittin commented on the pull request:
https://github.com/apache/incubator-brooklyn/pull/394#issuecomment-103071728
With this isn't it likely that when an entity is stopped someone might get the subscription manager populated with a non-deployment context where the `initialManagementContext` is the original context? And so calls to `subscribe` and `unsubscribe` will do something when we don't want them to?
Granted the code here has grown to be unwieldy -- the original intention was to allow a queuing context before the node is managed, which comes to life on management. But removing fail-fast behaviour doesn't seem like a good idea -- apart from for `unsubscribe` where it seems fine.
Does it make sense to change `isInitialManagementContextReal()` to `isInitialManagementContextUsable()` and in there put a check `if (mode==NonDeploymentManagementContextMode.MANAGEMENT_STOPPED) return false;` ?
And I think you're right, we should also guard `getSubscriptionManger()` with a `STOPPED` check, throwing or returning a non-deployment.
---
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] incubator-brooklyn pull request: Return null SubscriptionContext w...
Posted by ahgittin <gi...@git.apache.org>.
Github user ahgittin commented on the pull request:
https://github.com/apache/incubator-brooklyn/pull/394#issuecomment-155704502
@grkvlt Any thoughts on the above? We should update or close this PR!
---
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] incubator-brooklyn pull request: Return null SubscriptionContext w...
Posted by ahgittin <gi...@git.apache.org>.
Github user ahgittin commented on the pull request:
https://github.com/apache/incubator-brooklyn/pull/394#issuecomment-91034289
@grkvlt bump
---
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] incubator-brooklyn pull request: Return null SubscriptionContext w...
Posted by aledsage <gi...@git.apache.org>.
Github user aledsage commented on a diff in the pull request:
https://github.com/apache/incubator-brooklyn/pull/394#discussion_r22707761
--- Diff: core/src/main/java/brooklyn/management/internal/NonDeploymentManagementContext.java ---
@@ -227,8 +227,7 @@ public QueueingSubscriptionManager getSubscriptionManager() {
@Override
public synchronized SubscriptionContext getSubscriptionContext(Entity entity) {
if (!this.entity.equals(entity)) throw new IllegalStateException("Non-deployment context "+this+" can only use a single Entity: has "+this.entity+", but passed "+entity);
- if (mode==NonDeploymentManagementContextMode.MANAGEMENT_STOPPED)
- throw new IllegalStateException("Entity "+entity+" is no longer managed; subscription context not available");
+ if (mode==NonDeploymentManagementContextMode.MANAGEMENT_STOPPED) return null;
--- End diff --
I think I'd prefer that we return a `NonDeploymentSubscriptionContext`. That could throw exceptions on any attempt to subscribe etc, and would just `return false` for unsubscribe. I don't like having lots of null checks in the calling code.
---
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] incubator-brooklyn pull request: Return null SubscriptionContext w...
Posted by grkvlt <gi...@git.apache.org>.
Github user grkvlt commented on the pull request:
https://github.com/apache/incubator-brooklyn/pull/394#issuecomment-91034869
@ahgittin I'll change this to use a `NonDeploymentSubscriptionContext `
---
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] incubator-brooklyn pull request: Return null SubscriptionContext w...
Posted by grkvlt <gi...@git.apache.org>.
Github user grkvlt commented on the pull request:
https://github.com/apache/incubator-brooklyn/pull/394#issuecomment-94091722
@ahgittin If there are nofurther issues with this right now, I'll merge?
---
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] incubator-brooklyn pull request: Return null SubscriptionContext w...
Posted by grkvlt <gi...@git.apache.org>.
Github user grkvlt commented on the pull request:
https://github.com/apache/incubator-brooklyn/pull/394#issuecomment-67171654
@ahgittin Is this sensible? It was meant to stop errors being thrown when an entity is stopping, but perhaps the right fix is to work out why unsubscribe is being called after the management has been stopped?
---
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.
---