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/09/12 18:18:37 UTC
[GitHub] incubator-brooklyn pull request: BrooklynNode entity safe STOP eff...
GitHub user neykov opened a pull request:
https://github.com/apache/incubator-brooklyn/pull/161
BrooklynNode entity safe STOP effectors
Effectors to stop BrooklynNode forcing the user to choose explicitly whether to stop the running apps or not. Refuse to STOP if there are existing children (possibly mirroring nodes added by another entity)
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/neykov/incubator-brooklyn stop-brooklyn-node
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/incubator-brooklyn/pull/161.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 #161
----
commit e4369f06133fc8caafc1f1547071936640ef166f
Author: Svetoslav Neykov <sv...@cloudsoftcorp.com>
Date: 2014-09-12T15:53:05Z
Force UNIX-style line endings on shell scripts
commit 8eb6960896bd1284004c6b9c8b592c429ba4ca8a
Author: Svetoslav Neykov <sv...@cloudsoftcorp.com>
Date: 2014-09-12T16:02:55Z
HTTP API entity helpers
commit 5755227854a9969a1602efe835e75b5e0cd44a9b
Author: Svetoslav Neykov <sv...@cloudsoftcorp.com>
Date: 2014-09-12T16:07:50Z
Support for timeout parameter in the /server/shutdown request
commit f686ff3e23b71d02c3976fdcb04af1b39210d138
Author: Svetoslav Neykov <sv...@cloudsoftcorp.com>
Date: 2014-09-12T16:08:08Z
Shutdown effector in BrooklynNode
commit 1c6efeb6d48bfe5dba102ca47fb5958ae4297562
Author: Svetoslav Neykov <sv...@cloudsoftcorp.com>
Date: 2014-09-12T16:11:52Z
Brooklyn node safe STOP effectors
* Stop but leave apps running
* Stop apps and stop node
commit 809ecd5dac907f2d7446c67be0fb216698c2fcd7
Author: Svetoslav Neykov <sv...@cloudsoftcorp.com>
Date: 2014-09-12T16:13:35Z
Refuse stop of Brooklyn Node if there are child apps
(possibly mirrored child apps added manually)
----
---
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: BrooklynNode entity safe STOP eff...
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/161#discussion_r17719933
--- Diff: usage/rest-server/src/main/java/brooklyn/rest/resources/ServerResource.java ---
@@ -58,9 +62,13 @@ public void reloadBrooklynProperties() {
}
@Override
- public void shutdown(final boolean stopAppsFirst, final long delayMillis) {
+ public void shutdown(final boolean stopAppsFirst, final long delayMillis, String httpReturnTimeout) {
log.info("REST call to shutdown server, stopAppsFirst="+stopAppsFirst+", delayMillis="+delayMillis);
-
+
+ Duration httpReturnTimeoutDuration = Duration.parse(httpReturnTimeout);
--- End diff --
oh yeah, and there's another PR, i'll have a look at it shortly
---
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: BrooklynNode entity safe STOP eff...
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/161#discussion_r17530031
--- Diff: usage/rest-api/src/main/java/brooklyn/rest/api/ServerApi.java ---
@@ -53,7 +53,8 @@
@Consumes({MediaType.APPLICATION_FORM_URLENCODED})
public void shutdown(
@FormParam("stopAppsFirst") @DefaultValue("false") boolean stopAppsFirst,
- @FormParam("delayMillis") @DefaultValue("250") long delayMillis);
+ @FormParam("delayMillis") @DefaultValue("250") long delayMillis,
--- End diff --
oops sorry, for REST calls type-coercion won't apply. but the pattern elsewhere is to use strings and then `Time.parseTimeString`, as in `EffectorResource.invoke`
---
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: BrooklynNode entity safe STOP eff...
Posted by ahgittin <gi...@git.apache.org>.
Github user ahgittin commented on the pull request:
https://github.com/apache/incubator-brooklyn/pull/161#issuecomment-56021843
looks good and runs well. i've got some cleanups which make stop more reliable. once i've reviewed the associated PR (for shutdown) i'll merge this and add that and ping you for a quick scan of my changes.
---
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: BrooklynNode entity safe STOP eff...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/incubator-brooklyn/pull/161
---
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: BrooklynNode entity safe STOP eff...
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/161#discussion_r17724720
--- Diff: usage/rest-server/src/main/java/brooklyn/rest/resources/ServerResource.java ---
@@ -58,9 +62,13 @@ public void reloadBrooklynProperties() {
}
@Override
- public void shutdown(final boolean stopAppsFirst, final long delayMillis) {
+ public void shutdown(final boolean stopAppsFirst, final long delayMillis, String httpReturnTimeout) {
log.info("REST call to shutdown server, stopAppsFirst="+stopAppsFirst+", delayMillis="+delayMillis);
-
+
+ Duration httpReturnTimeoutDuration = Duration.parse(httpReturnTimeout);
--- End diff --
Minor point but I'd add a check in this code that explicitly treats blank as null. That feels safe here to me...
---
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: BrooklynNode entity safe STOP eff...
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/161#discussion_r17776848
--- Diff: software/base/src/main/java/brooklyn/entity/brooklynnode/EntityHttpClient.java ---
@@ -0,0 +1,60 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package brooklyn.entity.brooklynnode;
+
+import java.util.Map;
+
+import brooklyn.util.http.HttpTool;
+import brooklyn.util.http.HttpToolResponse;
+
+/**
+ * Helpful methods for making HTTP requests to {@link BrooklynNode} entities.
+ */
+public interface EntityHttpClient {
--- End diff --
thanks :)
---
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: BrooklynNode entity safe STOP eff...
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/161#discussion_r17530289
--- Diff: usage/rest-api/src/main/java/brooklyn/rest/api/ServerApi.java ---
@@ -53,7 +53,8 @@
@Consumes({MediaType.APPLICATION_FORM_URLENCODED})
public void shutdown(
@FormParam("stopAppsFirst") @DefaultValue("false") boolean stopAppsFirst,
- @FormParam("delayMillis") @DefaultValue("250") long delayMillis);
+ @FormParam("delayMillis") @DefaultValue("250") long delayMillis,
--- End diff --
and add `@ApiParam` ? i now appreciate that the API here previously took longs so you just followed that pattern, but i think we should move towards consistency. to preserve backwards compatibility we could have
@ApiParam(name = "delay", value = "Minimum delay before system.exit", required = false)
@FormParam("delay") @DefaultValue("250ms") String delay,
@ApiParam(name = "delayMillis", value = "Deprecated delay before timeout", required = false)
@FormParam("delayMillis") Long delayMillis
and then in the code have an `if (delayMillis!=null) { /* log warn of deprecated, and use this value */ }`
?
---
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: BrooklynNode entity safe STOP eff...
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/161#discussion_r17719851
--- Diff: software/base/src/test/java/brooklyn/entity/brooklynnode/BrooklynNodeIntegrationTest.java ---
@@ -427,6 +431,48 @@ public void testHttps() throws Exception {
}
}
+ public void testStopAndKillAppsEffector() throws Exception {
+ createNodeAndExecStopEffector(BrooklynNode.STOP_NODE_AND_KILL_APPS);
+ }
+
+ public void testStopButLeaveAppsEffector() throws Exception {
+ createNodeAndExecStopEffector(BrooklynNode.STOP_NODE_BUT_LEAVE_APPS);
+ }
+
+ private void createNodeAndExecStopEffector(Effector<?> eff) throws Exception {
--- End diff --
better now, `eff` is used :) however we still don't assert whether the app was stopped or not -- would be nice to test that, but not essential. (i've confirmed manually.)
---
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: BrooklynNode entity safe STOP eff...
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/161#discussion_r17529956
--- Diff: usage/rest-api/src/main/java/brooklyn/rest/api/ServerApi.java ---
@@ -53,7 +53,8 @@
@Consumes({MediaType.APPLICATION_FORM_URLENCODED})
public void shutdown(
@FormParam("stopAppsFirst") @DefaultValue("false") boolean stopAppsFirst,
- @FormParam("delayMillis") @DefaultValue("250") long delayMillis);
+ @FormParam("delayMillis") @DefaultValue("250") long delayMillis,
--- End diff --
could these be `Duration` ? type coercion should do the right thing converting.
---
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: BrooklynNode entity safe STOP eff...
Posted by neykov <gi...@git.apache.org>.
Github user neykov commented on the pull request:
https://github.com/apache/incubator-brooklyn/pull/161#issuecomment-55611130
Made the changes from the above comments (the shutdown proposal being partially implemented). Will create a new PR for the rest of the shutdown parameters.
---
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: BrooklynNode entity safe STOP eff...
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/161#discussion_r17776820
--- Diff: software/base/src/test/java/brooklyn/entity/brooklynnode/BrooklynNodeIntegrationTest.java ---
@@ -427,6 +431,48 @@ public void testHttps() throws Exception {
}
}
+ public void testStopAndKillAppsEffector() throws Exception {
+ createNodeAndExecStopEffector(BrooklynNode.STOP_NODE_AND_KILL_APPS);
+ }
+
+ public void testStopButLeaveAppsEffector() throws Exception {
+ createNodeAndExecStopEffector(BrooklynNode.STOP_NODE_BUT_LEAVE_APPS);
+ }
+
+ private void createNodeAndExecStopEffector(Effector<?> eff) throws Exception {
--- End diff --
The test models the logic in testCanStartAndStop where stopping the app is confirmed with SERVICE_UP == false. Nevertheless I am adding a check with getDriver().isRunning() (let's see what you think of 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: BrooklynNode entity safe STOP eff...
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/161#discussion_r17719728
--- Diff: core/src/main/java/brooklyn/util/http/HttpToolResponse.java ---
@@ -63,13 +64,19 @@ public HttpToolResponse(HttpResponse response, long startTime) {
try {
ByteArrayOutputStream out = new ByteArrayOutputStream();
- response.getEntity().getContentLength();
- durationMillisOfFirstResponse = System.currentTimeMillis() - startTime;
-
- ByteStreams.copy(response.getEntity().getContent(), out);
- content = out.toByteArray();
-
- response.getEntity().getContentLength();
+ HttpEntity entity = response.getEntity();
+ if (entity != null) {
+ entity.getContentLength();
+ durationMillisOfFirstResponse = System.currentTimeMillis() - startTime;
+
+ ByteStreams.copy(entity.getContent(), out);
+ content = out.toByteArray();
+
+ entity.getContentLength();
+ } else {
+ durationMillisOfFirstResponse = System.currentTimeMillis() - startTime;
--- End diff --
`Duration.since(startTime)`
---
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: BrooklynNode entity safe STOP eff...
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/161#discussion_r17530849
--- Diff: usage/rest-api/src/main/java/brooklyn/rest/api/ServerApi.java ---
@@ -53,7 +53,8 @@
@Consumes({MediaType.APPLICATION_FORM_URLENCODED})
public void shutdown(
@FormParam("stopAppsFirst") @DefaultValue("false") boolean stopAppsFirst,
- @FormParam("delayMillis") @DefaultValue("250") long delayMillis);
+ @FormParam("delayMillis") @DefaultValue("250") long delayMillis,
--- End diff --
dunno about you but i feel that the logic here is messy (my fault) and getting messier (your fault :) ) --
i wonder if the arguments and semantics should be something like:
* `stopAppsFirst` -- whether apps are stopped or left running (as is, default false)
* `forceShutdownOnError` -- whether to shutdown the system if apps fail to stop or timeout (default false)
* `shutdownTimeout` -- a maximum delay to wait for apps to gracefully stop before giving up or forcibly exiting (default 20s)
* `httpReturnTimeout` -- a maximum delay to wait for apps and mgmt context to stop before returning from the REST call; if this is >= the shutdownTimeout and the shutdownTimeout is exceeded the call will return with an error (and setting > shutdownTimeout has no difference to setting =shutdownTimeout) (default 20s)
* `delayForHttpReturn` -- a delay applied after the REST call is returning and before any `System.exit`, to permit the REST response to be returned (default 5s)
If there is an error stopping apps before restTimeout, or the httpReturnTimeout >= the shutdownTimeout and any app shutdown calls did not return, then the call returns an error. If force is true, it invokes the `mgmt.terminate` and `System.exit` sequence after `shutdownTimeout+delayForHttpReturn` the `delayForHttpReturn`.
Just thinking aloud ... wdyt? If you think this is overkill for now, maybe just paste these comments (or your thoughts on it) in the code for discussion at a later date.
---
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: BrooklynNode entity safe STOP eff...
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/161#discussion_r17724012
--- Diff: usage/rest-server/src/main/java/brooklyn/rest/resources/ServerResource.java ---
@@ -58,9 +62,13 @@ public void reloadBrooklynProperties() {
}
@Override
- public void shutdown(final boolean stopAppsFirst, final long delayMillis) {
+ public void shutdown(final boolean stopAppsFirst, final long delayMillis, String httpReturnTimeout) {
log.info("REST call to shutdown server, stopAppsFirst="+stopAppsFirst+", delayMillis="+delayMillis);
-
+
+ Duration httpReturnTimeoutDuration = Duration.parse(httpReturnTimeout);
--- End diff --
Duration.parse returns null on null input. Null value for httpReturnTimeout results in no request blocking, to preserve exiting behaviour (but the behaviour changes in #172).
Empty string will result in parsing error which I think is a good thing. Better to keep the behaviour identical to TypeCoercions logic. Though a Preconditions check would be better in this case, I will add the checks to #172.
---
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: BrooklynNode entity safe STOP eff...
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/161#discussion_r17530947
--- Diff: software/base/src/test/java/brooklyn/entity/brooklynnode/BrooklynNodeIntegrationTest.java ---
@@ -427,6 +431,48 @@ public void testHttps() throws Exception {
}
}
+ public void testStopAndKillAppsEffector() throws Exception {
--- End diff --
need `@Test` annotation (and on other methods)
---
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: BrooklynNode entity safe STOP eff...
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/161#discussion_r17719774
--- Diff: software/base/src/main/java/brooklyn/entity/brooklynnode/EntityHttpClient.java ---
@@ -0,0 +1,60 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package brooklyn.entity.brooklynnode;
+
+import java.util.Map;
+
+import brooklyn.util.http.HttpTool;
+import brooklyn.util.http.HttpToolResponse;
+
+/**
+ * Helpful methods for making HTTP requests to {@link BrooklynNode} entities.
+ */
+public interface EntityHttpClient {
--- End diff --
This is such an elegant solution. I just had to say that again.
---
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: BrooklynNode entity safe STOP eff...
Posted by ahgittin <gi...@git.apache.org>.
Github user ahgittin commented on the pull request:
https://github.com/apache/incubator-brooklyn/pull/161#issuecomment-56165889
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: BrooklynNode entity safe STOP eff...
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/161#discussion_r17530864
--- Diff: usage/rest-server/src/main/java/brooklyn/rest/resources/ServerResource.java ---
@@ -82,10 +87,32 @@ public void run() {
((ManagementContextInternal)mgmt()).terminate();
timer.waitForExpiryUnchecked();
-
+
+ synchronized (completed) {
+ completed.set(true);
+ completed.notifyAll();
+ }
+
+ if (shouldBlock) {
+ //give the http request a chance to complete gracefully
+ Duration.FIVE_SECONDS.countdownTimer().waitForExpiryUnchecked();
--- End diff --
can just say `Time.sleep(Duration.FIVE_SECONDS)` ?
---
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: BrooklynNode entity safe STOP eff...
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/161#discussion_r17529893
--- Diff: software/base/src/test/java/brooklyn/entity/brooklynnode/SameBrooklynNodeImpl.java ---
@@ -83,4 +83,9 @@ protected void disconnectSensors() {
if (httpFeed != null) httpFeed.stop();
}
+ @Override
+ public EntityHttpClient http() {
--- End diff --
i like the extraction of the `http()` as a convenience method. however would throwing `UnsupportedOperationException` be safer in this test class?
---
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: BrooklynNode entity safe STOP eff...
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/161#discussion_r17531028
--- Diff: software/base/src/test/java/brooklyn/entity/brooklynnode/BrooklynNodeIntegrationTest.java ---
@@ -427,6 +431,48 @@ public void testHttps() throws Exception {
}
}
+ public void testStopAndKillAppsEffector() throws Exception {
+ createNodeAndExecStopEffector(BrooklynNode.STOP_NODE_AND_KILL_APPS);
+ }
+
+ public void testStopButLeaveAppsEffector() throws Exception {
+ createNodeAndExecStopEffector(BrooklynNode.STOP_NODE_BUT_LEAVE_APPS);
+ }
+
+ private void createNodeAndExecStopEffector(Effector<?> eff) throws Exception {
--- End diff --
maybe replace lines 464 onwards with a simple `brooklynNode.invoke(eff, ...)`, return the task, and let the calling test inspect the tasks and children (adding a new `@Test testStopPlainThrowsException` to perform the existing test on lines 464-467) ?
---
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: BrooklynNode entity safe STOP eff...
Posted by ahgittin <gi...@git.apache.org>.
Github user ahgittin commented on the pull request:
https://github.com/apache/incubator-brooklyn/pull/161#issuecomment-55565083
completed review -- looks good overall but a few things to 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: BrooklynNode entity safe STOP eff...
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/161#discussion_r17550071
--- Diff: usage/rest-api/src/main/java/brooklyn/rest/api/ServerApi.java ---
@@ -53,7 +53,8 @@
@Consumes({MediaType.APPLICATION_FORM_URLENCODED})
public void shutdown(
@FormParam("stopAppsFirst") @DefaultValue("false") boolean stopAppsFirst,
- @FormParam("delayMillis") @DefaultValue("250") long delayMillis);
+ @FormParam("delayMillis") @DefaultValue("250") long delayMillis,
--- End diff --
Changed timeoutMillis to httpReturnTimeout so it is compatible with your proposal. I'd like to address the rest of the comment in a separate 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: BrooklynNode entity safe STOP eff...
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/161#discussion_r17777576
--- Diff: usage/rest-server/src/main/java/brooklyn/rest/resources/ServerResource.java ---
@@ -58,9 +62,13 @@ public void reloadBrooklynProperties() {
}
@Override
- public void shutdown(final boolean stopAppsFirst, final long delayMillis) {
+ public void shutdown(final boolean stopAppsFirst, final long delayMillis, String httpReturnTimeout) {
log.info("REST call to shutdown server, stopAppsFirst="+stopAppsFirst+", delayMillis="+delayMillis);
-
+
+ Duration httpReturnTimeoutDuration = Duration.parse(httpReturnTimeout);
--- End diff --
Addressed in #172
---
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: BrooklynNode entity safe STOP eff...
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/161#discussion_r17530953
--- Diff: software/base/src/test/java/brooklyn/entity/brooklynnode/BrooklynNodeIntegrationTest.java ---
@@ -427,6 +431,48 @@ public void testHttps() throws Exception {
}
}
+ public void testStopAndKillAppsEffector() throws Exception {
+ createNodeAndExecStopEffector(BrooklynNode.STOP_NODE_AND_KILL_APPS);
+ }
+
+ public void testStopButLeaveAppsEffector() throws Exception {
+ createNodeAndExecStopEffector(BrooklynNode.STOP_NODE_BUT_LEAVE_APPS);
+ }
+
+ private void createNodeAndExecStopEffector(Effector<?> eff) throws Exception {
--- End diff --
the `eff` parameter is ignored; we aren't testing what we thing we are 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: BrooklynNode entity safe STOP eff...
Posted by neykov <gi...@git.apache.org>.
Github user neykov commented on the pull request:
https://github.com/apache/incubator-brooklyn/pull/161#issuecomment-56159464
Addressed comments and finished 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: BrooklynNode entity safe STOP eff...
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/161#discussion_r17719919
--- Diff: usage/rest-server/src/main/java/brooklyn/rest/resources/ServerResource.java ---
@@ -58,9 +62,13 @@ public void reloadBrooklynProperties() {
}
@Override
- public void shutdown(final boolean stopAppsFirst, final long delayMillis) {
+ public void shutdown(final boolean stopAppsFirst, final long delayMillis, String httpReturnTimeout) {
log.info("REST call to shutdown server, stopAppsFirst="+stopAppsFirst+", delayMillis="+delayMillis);
-
+
+ Duration httpReturnTimeoutDuration = Duration.parse(httpReturnTimeout);
--- End diff --
`if (timeout==null || isEmpty) timeout="30s"` ? not needed tho....
---
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.
---