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 2014/10/21 03:54:17 UTC
[GitHub] incubator-brooklyn pull request: allow parameters for restart effe...
GitHub user ahgittin opened a pull request:
https://github.com/apache/incubator-brooklyn/pull/260
allow parameters for restart effector, including children and the machine for SoftwareProcess.restart;
if this model is sensible i'd like to apply it more widely to effectors, to allow parameters to tune behaviour.
(eg for stop, to allow controlling whether it is the machine or just the process which is stopped,
and on start, to allow forcing reinstall)
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/ahgittin/incubator-brooklyn restart-parameters
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/incubator-brooklyn/pull/260.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 #260
----
commit b12492bba71897cbfe0abcf7a32364a2e46072cd
Author: Alex Heneveld <al...@cloudsoftcorp.com>
Date: 2014-10-21T01:45:01Z
allow parameters for restart effector, including children and the machine for SoftwareProcess.restart;
if this model is sensible we might apply it more widely to effectors, to allow parameters to tune behaviour.
(eg for stop, to allow controlling whether it is the machine or just the process which is stopped,
and on start, to allow forcing reinstall)
----
---
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: allow parameters for restart effe...
Posted by sjcorbett <gi...@git.apache.org>.
Github user sjcorbett commented on a diff in the pull request:
https://github.com/apache/incubator-brooklyn/pull/260#discussion_r19137605
--- Diff: core/src/main/java/brooklyn/entity/trait/Startable.java ---
@@ -51,30 +56,68 @@
parameters.put(LOCATIONS, entity().getManagementContext().getLocationRegistry().resolveList(parameters.get(LOCATIONS)));
return new MethodEffector<Void>(Startable.class, "start").call(entity(), parameters.getAllConfig());
}
- };
-
+ }
+
+ public static class StopEffectorBody extends EffectorBody<Void> {
+ private static final Logger log = LoggerFactory.getLogger(Startable.class);
+
+ @Override public Void call(ConfigBag parameters) {
+ if (!parameters.isEmpty()) {
+ log.warn("Parameters "+parameters+" not supported for call to "+entity()+" - "+Tasks.current());
+ }
+
+ return new MethodEffector<Void>(Startable.class, "stop").call(entity(), parameters.getAllConfig());
+ }
+ }
+
+ public static class RestartEffectorBody extends EffectorBody<Void> {
+ private static final Logger log = LoggerFactory.getLogger(Startable.class);
+
+ @Override public Void call(ConfigBag parameters) {
+ if (!parameters.isEmpty()) {
+ log.warn("Parameters "+parameters+" not supported for call to "+entity()+" - "+Tasks.current());
+ }
+ return new MethodEffector<Void>(Startable.class, "restart").call(entity(), parameters.getAllConfig());
+ }
+ }
+
brooklyn.entity.Effector<Void> START = Effectors.effector(new MethodEffector<Void>(Startable.class, "start"))
// override start to take strings etc
.parameter(StartEffectorBody.LOCATIONS)
.impl(new StartEffectorBody())
.build();
- brooklyn.entity.Effector<Void> STOP = new MethodEffector<Void>(Startable.class, "stop");
- brooklyn.entity.Effector<Void> RESTART = new MethodEffector<Void>(Startable.class,"restart");
+
+ brooklyn.entity.Effector<Void> STOP = Effectors.effector(new MethodEffector<Void>(Startable.class, "stop"))
+ .impl(new StopEffectorBody())
+ .build();
+
+ brooklyn.entity.Effector<Void> RESTART = Effectors.effector(new MethodEffector<Void>(Startable.class, "restart"))
+ .impl(new RestartEffectorBody())
+ .build();
--- End diff --
I see this pattern used frequently but find it quite confusing. Why is the extra class of indirection (`*EffectorBody`) useful? So classes can replace `impl` with a class that acts on `parameters`? Why are those actions best done in an `EffectorBody` rather than the true effector method itself?
Line 69 calls:
```new MethodEffector<Void>(Startable.class, "stop").call(entity(), parameters.getAllConfig())```
Where is the definition of `stop` with a `Map<String, String>` argument? Or will the no-argument method be called magically?
---
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: allow parameters for restart effe...
Posted by ahgittin <gi...@git.apache.org>.
Github user ahgittin commented on the pull request:
https://github.com/apache/incubator-brooklyn/pull/260#issuecomment-60016035
at last, it passes all tests. @aledsage @richardcloudsoft i've been pondering for a while how to add parameters to `restart` and others, both what the code should look like and what the parameters should be. without parameters, the behaviour is ambiguous and often it's not what i want.
i think this solution is pretty good -- you have two optional parameters for `SoftwareProcess`, whether to restart the machine, and whether to restart children. the code is a little tricky but part of it is to support legacy paths (the methods in `SoftwareProcessImpl`, some of which have now been deprecated).
i can expect for `stop` we might have `stopProcess` `stopMachine` `stopChildren`.
i think on balance this is the direction we should go but wanted to put this out for feedback. (and for merging if y'all don't immediately hate it!)
---
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: allow parameters for restart effe...
Posted by rdowner <gi...@git.apache.org>.
Github user rdowner commented on a diff in the pull request:
https://github.com/apache/incubator-brooklyn/pull/260#discussion_r19336618
--- Diff: core/src/main/java/brooklyn/entity/trait/Startable.java ---
@@ -51,30 +56,68 @@
parameters.put(LOCATIONS, entity().getManagementContext().getLocationRegistry().resolveList(parameters.get(LOCATIONS)));
return new MethodEffector<Void>(Startable.class, "start").call(entity(), parameters.getAllConfig());
}
- };
-
+ }
+
+ public static class StopEffectorBody extends EffectorBody<Void> {
+ private static final Logger log = LoggerFactory.getLogger(Startable.class);
+
+ @Override public Void call(ConfigBag parameters) {
+ if (!parameters.isEmpty()) {
+ log.warn("Parameters "+parameters+" not supported for call to "+entity()+" - "+Tasks.current());
+ }
+
+ return new MethodEffector<Void>(Startable.class, "stop").call(entity(), parameters.getAllConfig());
+ }
+ }
+
+ public static class RestartEffectorBody extends EffectorBody<Void> {
+ private static final Logger log = LoggerFactory.getLogger(Startable.class);
+
+ @Override public Void call(ConfigBag parameters) {
+ if (!parameters.isEmpty()) {
+ log.warn("Parameters "+parameters+" not supported for call to "+entity()+" - "+Tasks.current());
+ }
+ return new MethodEffector<Void>(Startable.class, "restart").call(entity(), parameters.getAllConfig());
+ }
+ }
+
brooklyn.entity.Effector<Void> START = Effectors.effector(new MethodEffector<Void>(Startable.class, "start"))
// override start to take strings etc
.parameter(StartEffectorBody.LOCATIONS)
.impl(new StartEffectorBody())
.build();
- brooklyn.entity.Effector<Void> STOP = new MethodEffector<Void>(Startable.class, "stop");
- brooklyn.entity.Effector<Void> RESTART = new MethodEffector<Void>(Startable.class,"restart");
+
+ brooklyn.entity.Effector<Void> STOP = Effectors.effector(new MethodEffector<Void>(Startable.class, "stop"))
+ .impl(new StopEffectorBody())
+ .build();
+
+ brooklyn.entity.Effector<Void> RESTART = Effectors.effector(new MethodEffector<Void>(Startable.class, "restart"))
+ .impl(new RestartEffectorBody())
+ .build();
/**
* Start the entity in the given collection of locations.
+ * <p>
+ * Some entities may define custom {@link Effector} implementations which support
+ * a richer set of parameters. See the entity-specific {@link #START} effector declaration.
*/
@brooklyn.entity.annotation.Effector(description="Start the process/service represented by an entity")
void start(@EffectorParam(name="locations") Collection<? extends Location> locations);
/**
* Stop the entity.
+ * <p>
+ * Some entities may define custom {@link Effector} implementations which support
+ * a richer set of parameters. See the entity-specific {@link #START} effector declaration.
*/
--- End diff --
Should `{@link #START}` actually be `STOP`? Likewise for `RESTART` below?
---
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: allow parameters for restart effe...
Posted by rdowner <gi...@git.apache.org>.
Github user rdowner commented on the pull request:
https://github.com/apache/incubator-brooklyn/pull/260#issuecomment-60409129
I like the idea of clarifying exactly what the restart effector does, and in general I'm happy with this PR. However there's a few comments I'd like to see addressed before merging.
I'm also slightly concerned that we're changing the contract for downstream entities - I notice that a lot of entities have had to change `doStop()` to `postStop()`. What are all the implications for our user's entities?
Finally, a bit of a nitpicky point, but I'm not sure if I like `RestartMode.AUTO`. Can we pick a name that is more descriptive of what `AUTO` actually does?
---
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: allow parameters for restart effe...
Posted by rdowner <gi...@git.apache.org>.
Github user rdowner commented on a diff in the pull request:
https://github.com/apache/incubator-brooklyn/pull/260#discussion_r19347514
--- Diff: software/base/src/main/java/brooklyn/entity/software/MachineLifecycleEffectorTasks.java ---
@@ -418,28 +425,67 @@ protected void postStartCustom() {
// nothing by default
}
+ /** @deprecated since 0.7.0 use {@link #restart(ConfigBag)} */
+ @Deprecated
+ public void restart() {
+ restart(ConfigBag.EMPTY);
+ }
+
/**
* Default restart implementation for an entity.
* <p>
* Stops processes if possible, then starts the entity again.
*/
- public void restart() {
+ public void restart(ConfigBag parameters) {
ServiceStateLogic.setExpectedState(entity(), Lifecycle.STOPPING);
- DynamicTasks.queue("stopping (process)", new Callable<String>() { public String call() {
- DynamicTasks.markInessential();
- stopProcessesAtMachine();
- DynamicTasks.waitForLast();
- return "Stop of process completed with no errors.";
- }});
+
+ RestartMode isRestartMachine = parameters.get(RestartSoftwareParameters.RESTART_MACHINE_TYPED);
+ if (isRestartMachine==null) isRestartMachine=RestartMode.AUTO;
+ if (isRestartMachine==RestartMode.AUTO) isRestartMachine = RestartMode.FALSE;
--- End diff --
Why is `AUTO` not supported in this case? A comment would be helpful
---
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: allow parameters for restart effe...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/incubator-brooklyn/pull/260
---
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: allow parameters for restart effe...
Posted by ahgittin <gi...@git.apache.org>.
Github user ahgittin commented on the pull request:
https://github.com/apache/incubator-brooklyn/pull/260#issuecomment-60514335
great comments @rdowner, will address
---
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: allow parameters for restart effe...
Posted by ahgittin <gi...@git.apache.org>.
Github user ahgittin commented on the pull request:
https://github.com/apache/incubator-brooklyn/pull/260#issuecomment-60557950
i've changed the text and some naming around `RestartMode.AUTO` -- thanks @rdowner.
using `preStop()` or `postStop()` instead of `doStop()` was done on a case-by-case basis. we need to move away from overriding the `doXxx()` methods because they won't have the parameters passed in. it would be nice to be fully backwards compatible but this would make for ugliness i think (e.g. a `ThreadLocal parameters()` method?), so the better resolution i thought was to make it so incompatible usages are easily-discovered-and-fixed compilation errors.
addressed all the other comments, i think. i look forward to your re-review @rdowner !
---
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: allow parameters for restart effe...
Posted by rdowner <gi...@git.apache.org>.
Github user rdowner commented on a diff in the pull request:
https://github.com/apache/incubator-brooklyn/pull/260#discussion_r19336882
--- Diff: software/base/src/main/java/brooklyn/entity/basic/SoftwareProcess.java ---
@@ -233,4 +233,27 @@ private ChildStartableMode(boolean isDisabled, boolean isBackground, boolean isL
AttributeSensor<String> PID_FILE = Sensors.newStringSensor("softwareprocess.pid.file", "PID file");
+ public static class RestartSoftwareParameters {
+ @Beta /** @since 0.7.0 semantics of parameters to restart being explored */
+ public static final ConfigKey<Boolean> RESTART_CHILDREN = ConfigKeys.newConfigKey(Boolean.class, "restartChildren",
+ "Whether to restart children; default false", false);
+
+ @Beta /** @since 0.7.0 semantics of parameters to restart being explored */
+ public static final ConfigKey<Object> RESTART_MACHINE = ConfigKeys.newConfigKey(Object.class, "restartMachine",
+ "Whether to restart/replace the machine where this is running: 'true', 'false', or 'auto' supported, "
+ + "with the default being auto, ie only act at the machine level if there is nothing else to restart"
+ + " (e.g. there is no machine);"
+ + " NB: only applies when the machine was provisioned for this entity",
+ RestartMode.AUTO.toString().toLowerCase());
--- End diff --
The help text is not worded very well. Avoid "ie" and "NB"! Suggest:
`Whether to restart/replace the machine where this is running. Options are: 'true': <definition>; 'false': <definition>; 'auto': <definition>. Defaults to 'auto'.`
I don't understand your definition of `auto`, could you rewrite it? The highlighted parts seem to be contradictory:
only act at the **machine level** if there is nothing else to restart (e.g. there is **no machine**)
---
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: allow parameters for restart effe...
Posted by rdowner <gi...@git.apache.org>.
Github user rdowner commented on a diff in the pull request:
https://github.com/apache/incubator-brooklyn/pull/260#discussion_r19347153
--- Diff: software/base/src/main/java/brooklyn/entity/basic/SoftwareProcessImpl.java ---
@@ -471,24 +483,39 @@ public final void restart() {
/**
* To be overridden instead of {@link #start(Collection)}; sub-classes should call {@code super.doStart(locations)} and should
* add do additional work via tasks, executed using {@link DynamicTasks#queue(String, Callable)}.
+ * @deprecated since 0.7.0 override {@link #preStart()} or {@link #postStart()}, or define custom lifecycle tasks if needed
+ * (this method is no longer invoked, hence now marking it final)
*/
- protected void doStart(Collection<? extends Location> locations) {
+ @Deprecated
+ protected final void doStart(Collection<? extends Location> locations) {
LIFECYCLE_TASKS.start(locations);
}
/**
* To be overridden instead of {@link #stop()}; sub-classes should call {@code super.doStop()} and should
* add do additional work via tasks, executed using {@link DynamicTasks#queue(String, Callable)}.
+ * @deprecated since 0.7.0 override {@link #preStop()} or {@link #postStop()}, or define custom lifecycle tasks if needed
+ * (this method is no longer invoked, hence now marking it final)
*/
- protected void doStop() {
+ @Deprecated
+ protected final void doStop() {
LIFECYCLE_TASKS.stop();
}
/**
- * To be overridden instead of {@link #restart()}; sub-classes should call {@code super.doRestart()} and should
+ * To be overridden instead of {@link #restart()}; sub-classes should call {@code super.doRestart(ConfigBag)} and should
* add do additional work via tasks, executed using {@link DynamicTasks#queue(String, Callable)}.
+ * @deprecated since 0.7.0 define custom lifecycle tasks if needed
+ * (this method is no longer invoked, hence now marking it final)
*/
--- End diff --
Oh hang on, I may have mis-read the diff. Give me another minute.
---
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: allow parameters for restart effe...
Posted by ahgittin <gi...@git.apache.org>.
Github user ahgittin commented on the pull request:
https://github.com/apache/incubator-brooklyn/pull/260#issuecomment-60376305
@aledsage @richardcloudsoft comments?
---
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: allow parameters for restart effe...
Posted by rdowner <gi...@git.apache.org>.
Github user rdowner commented on a diff in the pull request:
https://github.com/apache/incubator-brooklyn/pull/260#discussion_r19336588
--- Diff: core/src/main/java/brooklyn/entity/trait/Startable.java ---
@@ -51,30 +56,68 @@
parameters.put(LOCATIONS, entity().getManagementContext().getLocationRegistry().resolveList(parameters.get(LOCATIONS)));
return new MethodEffector<Void>(Startable.class, "start").call(entity(), parameters.getAllConfig());
}
- };
-
+ }
+
+ public static class StopEffectorBody extends EffectorBody<Void> {
+ private static final Logger log = LoggerFactory.getLogger(Startable.class);
+
+ @Override public Void call(ConfigBag parameters) {
+ if (!parameters.isEmpty()) {
+ log.warn("Parameters "+parameters+" not supported for call to "+entity()+" - "+Tasks.current());
+ }
+
+ return new MethodEffector<Void>(Startable.class, "stop").call(entity(), parameters.getAllConfig());
+ }
+ }
+
+ public static class RestartEffectorBody extends EffectorBody<Void> {
+ private static final Logger log = LoggerFactory.getLogger(Startable.class);
+
+ @Override public Void call(ConfigBag parameters) {
+ if (!parameters.isEmpty()) {
+ log.warn("Parameters "+parameters+" not supported for call to "+entity()+" - "+Tasks.current());
+ }
+ return new MethodEffector<Void>(Startable.class, "restart").call(entity(), parameters.getAllConfig());
+ }
+ }
+
brooklyn.entity.Effector<Void> START = Effectors.effector(new MethodEffector<Void>(Startable.class, "start"))
// override start to take strings etc
.parameter(StartEffectorBody.LOCATIONS)
.impl(new StartEffectorBody())
.build();
- brooklyn.entity.Effector<Void> STOP = new MethodEffector<Void>(Startable.class, "stop");
- brooklyn.entity.Effector<Void> RESTART = new MethodEffector<Void>(Startable.class,"restart");
+
+ brooklyn.entity.Effector<Void> STOP = Effectors.effector(new MethodEffector<Void>(Startable.class, "stop"))
+ .impl(new StopEffectorBody())
+ .build();
+
+ brooklyn.entity.Effector<Void> RESTART = Effectors.effector(new MethodEffector<Void>(Startable.class, "restart"))
+ .impl(new RestartEffectorBody())
+ .build();
--- End diff --
Have to agree with @sjcorbett on this one. Seems like this is adding a lot of SLOC just to check and warn about one condition.
It looks like it should be possible to refactor `StopEffectorBody` and `RestartEffectorBody` into a general-purpose `WarnIfParametersGivenEffectorWrapper`? It seems that these are trying to wrap another effector body, so it would be informative to put `Wrapper` into the class name somewhere. I can then see a static class `EffectorWrapper` or `EffectorBodies` that holds a useful library of effector bodies, along the line of `Predicates` or `Functions`.
---
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: allow parameters for restart effe...
Posted by rdowner <gi...@git.apache.org>.
Github user rdowner commented on a diff in the pull request:
https://github.com/apache/incubator-brooklyn/pull/260#discussion_r19347119
--- Diff: software/base/src/main/java/brooklyn/entity/basic/SoftwareProcessImpl.java ---
@@ -471,24 +483,39 @@ public final void restart() {
/**
* To be overridden instead of {@link #start(Collection)}; sub-classes should call {@code super.doStart(locations)} and should
* add do additional work via tasks, executed using {@link DynamicTasks#queue(String, Callable)}.
+ * @deprecated since 0.7.0 override {@link #preStart()} or {@link #postStart()}, or define custom lifecycle tasks if needed
+ * (this method is no longer invoked, hence now marking it final)
*/
- protected void doStart(Collection<? extends Location> locations) {
+ @Deprecated
+ protected final void doStart(Collection<? extends Location> locations) {
LIFECYCLE_TASKS.start(locations);
}
/**
* To be overridden instead of {@link #stop()}; sub-classes should call {@code super.doStop()} and should
* add do additional work via tasks, executed using {@link DynamicTasks#queue(String, Callable)}.
+ * @deprecated since 0.7.0 override {@link #preStop()} or {@link #postStop()}, or define custom lifecycle tasks if needed
+ * (this method is no longer invoked, hence now marking it final)
*/
- protected void doStop() {
+ @Deprecated
+ protected final void doStop() {
LIFECYCLE_TASKS.stop();
}
/**
- * To be overridden instead of {@link #restart()}; sub-classes should call {@code super.doRestart()} and should
+ * To be overridden instead of {@link #restart()}; sub-classes should call {@code super.doRestart(ConfigBag)} and should
* add do additional work via tasks, executed using {@link DynamicTasks#queue(String, Callable)}.
+ * @deprecated since 0.7.0 define custom lifecycle tasks if needed
+ * (this method is no longer invoked, hence now marking it final)
*/
--- End diff --
Probably breaking deprecation policy here:
1. By marking it final, we've caused compile errors in any downstream code that tried to override it
2. Even if it wasn't final, since nothing calls it any more, we've broken the contract with any code that tried to override it
If we can't follow the deprecation policy and we need to make an exception here, then I think the exception should be to just remove it completely?
---
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: allow parameters for restart effe...
Posted by rdowner <gi...@git.apache.org>.
Github user rdowner commented on a diff in the pull request:
https://github.com/apache/incubator-brooklyn/pull/260#discussion_r19347423
--- Diff: software/base/src/main/java/brooklyn/entity/basic/SoftwareProcessImpl.java ---
@@ -471,24 +483,39 @@ public final void restart() {
/**
* To be overridden instead of {@link #start(Collection)}; sub-classes should call {@code super.doStart(locations)} and should
* add do additional work via tasks, executed using {@link DynamicTasks#queue(String, Callable)}.
+ * @deprecated since 0.7.0 override {@link #preStart()} or {@link #postStart()}, or define custom lifecycle tasks if needed
+ * (this method is no longer invoked, hence now marking it final)
*/
- protected void doStart(Collection<? extends Location> locations) {
+ @Deprecated
+ protected final void doStart(Collection<? extends Location> locations) {
LIFECYCLE_TASKS.start(locations);
}
/**
* To be overridden instead of {@link #stop()}; sub-classes should call {@code super.doStop()} and should
* add do additional work via tasks, executed using {@link DynamicTasks#queue(String, Callable)}.
+ * @deprecated since 0.7.0 override {@link #preStop()} or {@link #postStop()}, or define custom lifecycle tasks if needed
+ * (this method is no longer invoked, hence now marking it final)
*/
- protected void doStop() {
+ @Deprecated
+ protected final void doStop() {
LIFECYCLE_TASKS.stop();
}
/**
- * To be overridden instead of {@link #restart()}; sub-classes should call {@code super.doRestart()} and should
+ * To be overridden instead of {@link #restart()}; sub-classes should call {@code super.doRestart(ConfigBag)} and should
* add do additional work via tasks, executed using {@link DynamicTasks#queue(String, Callable)}.
+ * @deprecated since 0.7.0 define custom lifecycle tasks if needed
+ * (this method is no longer invoked, hence now marking it final)
*/
--- End diff --
Right, my original comment stands - plus there's a brand new method which has been immediately deprecated, where the Javadoc states nothing invokes it yet something does seem to invoke it earlier in the class. Meanwhile, the existing method has had it's Javadoc removed. Either it's too late in the week for me to be able to read diffs (which is completely plausible :weary:) or this section needs revisiting?
---
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: allow parameters for restart effe...
Posted by rdowner <gi...@git.apache.org>.
Github user rdowner commented on a diff in the pull request:
https://github.com/apache/incubator-brooklyn/pull/260#discussion_r19346648
--- Diff: software/base/src/main/java/brooklyn/entity/basic/SoftwareProcessDriverLifecycleEffectorTasks.java ---
@@ -43,24 +46,35 @@
private static final Logger log = LoggerFactory.getLogger(SoftwareProcessDriverLifecycleEffectorTasks.class);
- @Override
- public void restart() {
- // children are ignored during restart currently - see ChildStartableMode
+ public void restart(ConfigBag parameters) {
+ RestartMode isRestartMachine = parameters.get(RestartSoftwareParameters.RESTART_MACHINE_TYPED);
+ if (isRestartMachine==null) isRestartMachine=RestartMode.AUTO;
- if (entity().getDriver() == null) {
- log.debug("restart of "+entity()+" has no driver - doing machine-level restart");
- super.restart();
+ if (isRestartMachine==RestartMode.TRUE) {
+ log.debug("restart of "+entity()+" has no driver - machine restart requested");
+ super.restart(parameters);
return;
}
- if (Strings.isEmpty(entity().getAttribute(Attributes.HOSTNAME))) {
- log.debug("restart of "+entity()+" has no hostname - doing machine-level restart");
- super.restart();
- return;
+ if (isRestartMachine==RestartMode.AUTO) {
+ if (entity().getDriver() == null) {
+ log.debug("restart of "+entity()+" has no driver - doing machine-level restart");
+ super.restart(parameters);
+ return;
+ }
+
+ if (Strings.isEmpty(entity().getAttribute(Attributes.HOSTNAME))) {
+ log.debug("restart of "+entity()+" has no hostname - doing machine-level restart");
+ super.restart(parameters);
+ return;
+ }
}
--- End diff --
Could this cause a machine to be rebooted twice? If this is logically impossible then a comment to make that clear would be helpful.
---
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: allow parameters for restart effe...
Posted by ahgittin <gi...@git.apache.org>.
Github user ahgittin commented on a diff in the pull request:
https://github.com/apache/incubator-brooklyn/pull/260#discussion_r19390844
--- Diff: core/src/main/java/brooklyn/entity/trait/Startable.java ---
@@ -51,30 +56,68 @@
parameters.put(LOCATIONS, entity().getManagementContext().getLocationRegistry().resolveList(parameters.get(LOCATIONS)));
return new MethodEffector<Void>(Startable.class, "start").call(entity(), parameters.getAllConfig());
}
- };
-
+ }
+
+ public static class StopEffectorBody extends EffectorBody<Void> {
+ private static final Logger log = LoggerFactory.getLogger(Startable.class);
+
+ @Override public Void call(ConfigBag parameters) {
+ if (!parameters.isEmpty()) {
+ log.warn("Parameters "+parameters+" not supported for call to "+entity()+" - "+Tasks.current());
+ }
+
+ return new MethodEffector<Void>(Startable.class, "stop").call(entity(), parameters.getAllConfig());
+ }
+ }
+
+ public static class RestartEffectorBody extends EffectorBody<Void> {
+ private static final Logger log = LoggerFactory.getLogger(Startable.class);
+
+ @Override public Void call(ConfigBag parameters) {
+ if (!parameters.isEmpty()) {
+ log.warn("Parameters "+parameters+" not supported for call to "+entity()+" - "+Tasks.current());
+ }
+ return new MethodEffector<Void>(Startable.class, "restart").call(entity(), parameters.getAllConfig());
+ }
+ }
+
brooklyn.entity.Effector<Void> START = Effectors.effector(new MethodEffector<Void>(Startable.class, "start"))
// override start to take strings etc
.parameter(StartEffectorBody.LOCATIONS)
.impl(new StartEffectorBody())
.build();
- brooklyn.entity.Effector<Void> STOP = new MethodEffector<Void>(Startable.class, "stop");
- brooklyn.entity.Effector<Void> RESTART = new MethodEffector<Void>(Startable.class,"restart");
+
+ brooklyn.entity.Effector<Void> STOP = Effectors.effector(new MethodEffector<Void>(Startable.class, "stop"))
+ .impl(new StopEffectorBody())
+ .build();
+
+ brooklyn.entity.Effector<Void> RESTART = Effectors.effector(new MethodEffector<Void>(Startable.class, "restart"))
+ .impl(new RestartEffectorBody())
+ .build();
--- End diff --
the approach here moves away from using the methods as the implementation of the effector, primarily to support optional parameters.
the complexity comes because we already had the methods, and they are useful for callers in a java context. but my preference would be to move away from methods as effectors altogether, so all effector operations become first class objects. it is more weight, having classes for each, but the benefits outweigh it in my view: it makes it easy to add parameters, and it makes it easy to apply effector implementations to multiple entities. (we could even get rid of the EntityProxy!)
so while you're right that this `StopEffectorBody` could be replaced with a general `WarnIfParametersGivenEffectorWrapper`, the intention is that a subclass entity could redefine the stop effector to have a subclass which does more interesting things with effectors.
---
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: allow parameters for restart effe...
Posted by rdowner <gi...@git.apache.org>.
Github user rdowner commented on the pull request:
https://github.com/apache/incubator-brooklyn/pull/260#issuecomment-60572509
I'm happier with your explanations and revised code comments. To be clear (correct me if I am wrong), we are breaking our deprecation policies with this PR by deprecating something and then immediately making it not work (causing compiler errors) - I think once merged a post to dev@brooklyn list would be a good idea to make sure people know to expect this type of compilation failure.
I'd suggest squashing the "code review" commit, but if that's not easily done, then I'm happy for you to merge as is.
---
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.
---