You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@brooklyn.apache.org by neykov <gi...@git.apache.org> on 2014/12/23 13:36:32 UTC
[GitHub] incubator-brooklyn pull request: Add finer-grained STOP effector p...
GitHub user neykov opened a pull request:
https://github.com/apache/incubator-brooklyn/pull/422
Add finer-grained STOP effector parameters.
Separate flags for process and machine stop, better control with regard to the machine state.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/neykov/incubator-brooklyn finegrained-stop-params
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/incubator-brooklyn/pull/422.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 #422
----
commit 05cee16c3493579f77dca540ae5b69fa4352d79a
Author: Svetoslav Neykov <sv...@cloudsoftcorp.com>
Date: 2014-12-23T12:35:04Z
Add finer-grained STOP effector parameters.
Separate flags for process and machine stop, better control with regard to the machine state.
----
---
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: Finer-grained STOP effector param...
Posted by aledsage <gi...@git.apache.org>.
Github user aledsage commented on the pull request:
https://github.com/apache/incubator-brooklyn/pull/422#issuecomment-70324127
@neykov Looks good; needs the test fixed in `MachineLifecycleEffectorTasksTest.java` and then can be merged.
---
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: Finer-grained STOP effector param...
Posted by neykov <gi...@git.apache.org>.
Github user neykov commented on the pull request:
https://github.com/apache/incubator-brooklyn/pull/422#issuecomment-69007332
Addressed 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: Finer-grained STOP effector param...
Posted by sjcorbett <gi...@git.apache.org>.
Github user sjcorbett commented on the pull request:
https://github.com/apache/incubator-brooklyn/pull/422#issuecomment-69016693
I think your assertions are the wrong way round. Did you mean to `assertEquals(canStop, !expected)`?
---
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: Finer-grained STOP effector param...
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/422#discussion_r22469386
--- Diff: software/base/src/test/java/brooklyn/entity/basic/SoftwareProcessEntityTest.java ---
@@ -267,6 +275,87 @@ public void testBasicSoftwareProcessStopsProcess() throws Exception {
}
@Test
+ public void testBasicSoftwareProcessCanStop() {
+ Object[] matrix = new Object[] {
+ StopMode.ALWAYS, true, true,
+ StopMode.ALWAYS, false, true,
+ StopMode.IF_NOT_STOPPED, true, false,
+ StopMode.IF_NOT_STOPPED, false, true,
+ StopMode.NEVER, true, false,
+ StopMode.NEVER, false, false
+ };
+ for (int i = 0; i < matrix.length; i+=3) {
+ StopMode mode = (StopMode)matrix[i];
+ boolean isEntityStopped = (Boolean)matrix[i+1];
+ boolean expected = (Boolean)matrix[i+2];
+ boolean canStop = MachineLifecycleEffectorTasks.canStop(mode, isEntityStopped);
+ assertEquals(canStop, expected);
--- End diff --
Might be useful to add a message to the assert indicating the mode if we expect this to fail. It's overkill for this test but you could also use TestNG's [data providers](http://testng.org/doc/documentation-main.html#parameters-dataproviders).
---
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: Finer-grained STOP effector param...
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/422#discussion_r22468714
--- Diff: software/base/src/main/java/brooklyn/entity/software/MachineLifecycleEffectorTasks.java ---
@@ -76,6 +77,7 @@
import brooklyn.util.text.Strings;
import brooklyn.util.time.Duration;
+import com.google.api.client.repackaged.com.google.common.annotations.VisibleForTesting;
--- End diff --
Use `com.google.common.annotations.VisibleForTesting` instead.
---
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: Finer-grained STOP effector param...
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/422#discussion_r22470155
--- Diff: software/base/src/main/java/brooklyn/entity/basic/SoftwareProcess.java ---
@@ -261,6 +261,20 @@ private ChildStartableMode(boolean isDisabled, boolean isBackground, boolean isL
public static final ConfigKey<Boolean> STOP_MACHINE = ConfigKeys.newBooleanConfigKey("stopMachine",
"Whether to stop the machine provisioned for this entity: 'true', or 'false' are supported, "
+ "with the default being 'true'", true);
+
+ //IF_NOT_STOPPED includes STARTING, STOPPING, RUNNING
+ public enum StopMode { ALWAYS, IF_NOT_STOPPED, NEVER };
+
+ @Beta /** @since 0.7.0 semantics of parameters to restart being explored */
+ public static final ConfigKey<StopMode> STOP_PROCESS_MODE = ConfigKeys.newConfigKey(StopMode.class, "stopProcessMode",
+ "When to stop the process with regard to the entity state", StopMode.IF_NOT_STOPPED);
+
+ @Beta /** @since 0.7.0 semantics of parameters to restart being explored */
+ public static final ConfigKey<StopMode> STOP_MACHINE_MODE = ConfigKeys.newConfigKey(StopMode.class, "stopMachineMode",
+ "When to stop the machine with regard to the entity state. " +
+ "ALWAYS will try to stop the machine even if the entity is already stopped," +
+ "IF_NOT_STOPPED stops the machine only if entity not already stopped, " +
--- End diff --
if the entity 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.
---
[GitHub] incubator-brooklyn pull request: Finer-grained STOP effector param...
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/422#discussion_r23107887
--- Diff: software/base/src/test/java/brooklyn/entity/software/MachineLifecycleEffectorTasksTest.java ---
@@ -0,0 +1,33 @@
+package brooklyn.entity.software;
+
+import static org.testng.Assert.assertEquals;
+
+import org.testng.annotations.DataProvider;
+import org.testng.annotations.Test;
+
+import brooklyn.entity.basic.SoftwareProcess.StopSoftwareParameters.StopMode;
+
+public class MachineLifecycleEffectorTasksTest {
+ public static boolean canStop(StopMode stopMode, boolean isEntityStopped) {
+ return MachineLifecycleEffectorTasks.canStop(stopMode, isEntityStopped);
+ }
+
+ @DataProvider(name = "canStopStates")
+ public Object[][] canStopStates() {
+ return new Object[][] {
+ { StopMode.ALWAYS, true, true },
+ { StopMode.ALWAYS, false, true },
+ { StopMode.IF_NOT_STOPPED, true, false },
+ { StopMode.IF_NOT_STOPPED, false, true },
+ { StopMode.NEVER, true, false },
+ { StopMode.NEVER, false, false },
+ };
+ }
+
+ @Test(dataProvider = "canStopStates")
+ public void testBasicSoftwareProcessCanStop(StopMode mode, boolean isEntityStopped, boolean expected) {
+ boolean canStop = canStop(mode, isEntityStopped);
+ assertEquals(canStop, !expected);
--- End diff --
I agree with @sjcorbett and checked by running test: this should be `assertEquals(canStop, expected)`.
---
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: Finer-grained STOP effector param...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/incubator-brooklyn/pull/422
---
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: Finer-grained STOP effector param...
Posted by neykov <gi...@git.apache.org>.
Github user neykov commented on the pull request:
https://github.com/apache/incubator-brooklyn/pull/422#issuecomment-70330989
Fixed test.
---
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: Finer-grained STOP effector param...
Posted by sjcorbett <gi...@git.apache.org>.
Github user sjcorbett commented on the pull request:
https://github.com/apache/incubator-brooklyn/pull/422#issuecomment-68732292
Looks great. A few minor comments. Confirmed that unit and integration tests pass.
---
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: Finer-grained STOP effector param...
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/422#discussion_r22468710
--- Diff: software/base/src/main/java/brooklyn/entity/basic/SoftwareProcess.java ---
@@ -261,6 +261,20 @@ private ChildStartableMode(boolean isDisabled, boolean isBackground, boolean isL
public static final ConfigKey<Boolean> STOP_MACHINE = ConfigKeys.newBooleanConfigKey("stopMachine",
"Whether to stop the machine provisioned for this entity: 'true', or 'false' are supported, "
+ "with the default being 'true'", true);
+
+ //IF_NOT_STOPPED includes STARTING, STOPPING, RUNNING
+ public enum StopMode { ALWAYS, IF_NOT_STOPPED, NEVER };
+
+ @Beta /** @since 0.7.0 semantics of parameters to restart being explored */
+ public static final ConfigKey<StopMode> STOP_PROCESS_MODE = ConfigKeys.newConfigKey(StopMode.class, "stopProcessMode",
+ "When to stop the process with regard to the entity state", StopMode.IF_NOT_STOPPED);
+
+ @Beta /** @since 0.7.0 semantics of parameters to restart being explored */
+ public static final ConfigKey<StopMode> STOP_MACHINE_MODE = ConfigKeys.newConfigKey(StopMode.class, "stopMachineMode",
+ "When to stop the machine with regard to the entity state. " +
+ "ALWAYS will try to stop the machine even if the entity is already stopped," +
+ "IF_NOT_STOPPED stops the machine only if entity not already stopped, " +
+ "NEVER doesn't stop the machine. ", StopMode.IF_NOT_STOPPED);
--- End diff --
Trailing space.
---
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: Finer-grained STOP effector param...
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/422#discussion_r22469063
--- Diff: software/base/src/main/java/brooklyn/entity/software/MachineLifecycleEffectorTasks.java ---
@@ -614,6 +631,23 @@ public void stop(ConfigBag parameters) {
if (log.isDebugEnabled()) log.debug("Stopped software process entity "+entity());
}
+ @VisibleForTesting
+ public static boolean canStop(StopMode stopMode, boolean isEntityStopped) {
--- End diff --
May as well reduce the visibility of the method if it's only exposed for testing.
---
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: Finer-grained STOP effector param...
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/422#discussion_r22468677
--- Diff: software/base/src/main/java/brooklyn/entity/basic/SoftwareProcess.java ---
@@ -261,6 +261,20 @@ private ChildStartableMode(boolean isDisabled, boolean isBackground, boolean isL
public static final ConfigKey<Boolean> STOP_MACHINE = ConfigKeys.newBooleanConfigKey("stopMachine",
"Whether to stop the machine provisioned for this entity: 'true', or 'false' are supported, "
+ "with the default being 'true'", true);
+
+ //IF_NOT_STOPPED includes STARTING, STOPPING, RUNNING
+ public enum StopMode { ALWAYS, IF_NOT_STOPPED, NEVER };
+
+ @Beta /** @since 0.7.0 semantics of parameters to restart being explored */
+ public static final ConfigKey<StopMode> STOP_PROCESS_MODE = ConfigKeys.newConfigKey(StopMode.class, "stopProcessMode",
+ "When to stop the process with regard to the entity state", StopMode.IF_NOT_STOPPED);
+
+ @Beta /** @since 0.7.0 semantics of parameters to restart being explored */
+ public static final ConfigKey<StopMode> STOP_MACHINE_MODE = ConfigKeys.newConfigKey(StopMode.class, "stopMachineMode",
+ "When to stop the machine with regard to the entity state. " +
+ "ALWAYS will try to stop the machine even if the entity is already stopped," +
--- End diff --
Add a space at the end of the line.
---
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: Finer-grained STOP effector param...
Posted by neykov <gi...@git.apache.org>.
Github user neykov commented on a diff in the pull request:
https://github.com/apache/incubator-brooklyn/pull/422#discussion_r22580886
--- Diff: software/base/src/test/java/brooklyn/entity/basic/SoftwareProcessEntityTest.java ---
@@ -267,6 +275,87 @@ public void testBasicSoftwareProcessStopsProcess() throws Exception {
}
@Test
+ public void testBasicSoftwareProcessCanStop() {
+ Object[] matrix = new Object[] {
+ StopMode.ALWAYS, true, true,
+ StopMode.ALWAYS, false, true,
+ StopMode.IF_NOT_STOPPED, true, false,
+ StopMode.IF_NOT_STOPPED, false, true,
+ StopMode.NEVER, true, false,
+ StopMode.NEVER, false, false
+ };
+ for (int i = 0; i < matrix.length; i+=3) {
+ StopMode mode = (StopMode)matrix[i];
+ boolean isEntityStopped = (Boolean)matrix[i+1];
+ boolean expected = (Boolean)matrix[i+2];
+ boolean canStop = MachineLifecycleEffectorTasks.canStop(mode, isEntityStopped);
+ assertEquals(canStop, expected);
--- End diff --
Thanks for the data providers pointer, changed the code to use them. Logs the failing mode parameters automatically.
---
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: Finer-grained STOP effector param...
Posted by neykov <gi...@git.apache.org>.
Github user neykov commented on a diff in the pull request:
https://github.com/apache/incubator-brooklyn/pull/422#discussion_r22580823
--- Diff: software/base/src/main/java/brooklyn/entity/software/MachineLifecycleEffectorTasks.java ---
@@ -614,6 +631,23 @@ public void stop(ConfigBag parameters) {
if (log.isDebugEnabled()) log.debug("Stopped software process entity "+entity());
}
+ @VisibleForTesting
+ public static boolean canStop(StopMode stopMode, boolean isEntityStopped) {
--- End diff --
The method is used in a test from a different package, that's why the public visibility is needed.
I changed the code a bit - the method is now protected with the test code providing a public wrapper, no need for the annotation. wdyt?
---
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: Finer-grained STOP effector param...
Posted by neykov <gi...@git.apache.org>.
Github user neykov commented on the pull request:
https://github.com/apache/incubator-brooklyn/pull/422#issuecomment-70333335
Not clear what went wrong with the last build - all projects are SUCCESS. Rebasing to force another build.
---
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.
---