You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@brooklyn.apache.org by aledsage <gi...@git.apache.org> on 2014/10/29 14:56:35 UTC
[GitHub] incubator-brooklyn pull request: Fix: various 20141028
GitHub user aledsage opened a pull request:
https://github.com/apache/incubator-brooklyn/pull/280
Fix: various 20141028
See more extensive comments in each commit that says what it is for.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/aledsage/incubator-brooklyn fix/various-20141028
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/incubator-brooklyn/pull/280.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 #280
----
commit 3f9ffe49c1348e97f8005898cd8a37ebbda41556
Author: Aled Sage <al...@gmail.com>
Date: 2014-10-28T18:56:03Z
Fix typos in chef-blueprints.md
commit e9b22a4188c1b0af26c38c63500cbd28e6177727
Author: Aled Sage <al...@gmail.com>
Date: 2014-10-28T19:20:16Z
Entities.waitForServiceUp: use finally block
commit a3a11927f9f40d4fc8488a7be65154c3919ab843
Author: Aled Sage <al...@gmail.com>
Date: 2014-10-28T23:33:01Z
Adds MutableList.builder.addAll(V[])
commit b8508ed5e73df41c8e5586db33b1d7eed50e8711
Author: Aled Sage <al...@gmail.com>
Date: 2014-10-29T10:57:45Z
Fix BrooklynNode.stop
- For check that children.isEmpty + shutdown call, can’t just check
serviceUp=true because earlier in stop it will have unset that.
Therefore guard with webConsoleAccessible=false.
- For kill, wait for webConsoleAccessible=false before calling
stop(), so that it doesn’t fail on children.isEmpty
- In testStopPlainThrowsException, don’t let errors on startup
have the assertion about exceptions pass!
commit 0fc4a4f327686757e7e8c25a3ec247796713105b
Author: Aled Sage <al...@gmail.com>
Date: 2014-10-29T10:59:57Z
Fix ElasticSearch test
- Support for OS X: don’t fail if can’t install latest Java
- Accept documentCount > 1 across all nodes.
Is this because of a replica?
- Don’t expect the node where the put was executed to be the one
with the document.
commit 389fb5ca2e6698e714bdeba33662afb3b9e5b6d4
Author: Aled Sage <al...@gmail.com>
Date: 2014-10-29T12:36:15Z
Fix RedisIntegrationTest
- increase timeout, because on jenkins sometimes it takes crazy long
(e.g. 26 seconds) for `info server` to return.
----
---
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: Fix: various 20141028
Posted by aledsage <gi...@git.apache.org>.
Github user aledsage commented on the pull request:
https://github.com/apache/incubator-brooklyn/pull/280#issuecomment-60954472
Thanks; merging.
---
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: Fix: various 20141028
Posted by alasdairhodge <gi...@git.apache.org>.
Github user alasdairhodge commented on a diff in the pull request:
https://github.com/apache/incubator-brooklyn/pull/280#discussion_r19539523
--- Diff: utils/common/src/main/java/brooklyn/util/collections/MutableList.java ---
@@ -154,6 +154,13 @@ public Builder() {}
return this;
}
+ public Builder<V> addAll(V[] vals) {
+ for (V v : vals) {
+ result.add(v);
+ }
+ return this;
+ }
+
--- End diff --
Should add tests for this and other builder methods to `MutableListTest`.
---
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: Fix: various 20141028
Posted by alasdairhodge <gi...@git.apache.org>.
Github user alasdairhodge commented on the pull request:
https://github.com/apache/incubator-brooklyn/pull/280#issuecomment-60936324
Minor niggles, but looks sensible overall.
---
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: Fix: various 20141028
Posted by alasdairhodge <gi...@git.apache.org>.
Github user alasdairhodge commented on a diff in the pull request:
https://github.com/apache/incubator-brooklyn/pull/280#discussion_r19541273
--- Diff: software/base/src/test/java/brooklyn/entity/brooklynnode/BrooklynNodeIntegrationTest.java ---
@@ -434,11 +435,24 @@ public void testAuthenticationAndHttps() throws Exception {
Assert.assertEquals(response.getResponseCode(), 200);
}
- @Test(groups="Integration", expectedExceptions = PropagatedRuntimeException.class)
+ @Test(groups="Integration")
public void testStopPlainThrowsException() throws Exception {
BrooklynNode brooklynNode = setUpBrooklynNodeWithApp();
- brooklynNode.stop();
+ // Not using annotation with `expectedExceptions = PropagatedRuntimeException.class` because want to
+ // ensure exception comes from stop. On jenkins, was seeing setUpBrooklynNodeWithApp fail in
+ // testStopAndKillAppsEffector; so can't tell if this method was really passing!
+ try {
+ brooklynNode.stop();
+ fail("Expected "+brooklynNode+" stop to fail, because has app");
+ } catch (Exception e) {
+ IllegalStateException ise = Exceptions.getFirstThrowableOfType(e, IllegalStateException.class);
+ if (ise != null && ise.toString().contains("Can't stop instance with running applications")) {
+ // success
+ } else {
+ throw e;
+ }
+ }
--- End diff --
I see the intention here, but testing for explicit strings in exception messages always feels messy. But then so does `expectedExceptions=PropagatedRuntimeException.class`. While it's arguably neater to throw (and expect) a custom runtime exception type, since this is a very narrow test for the class that defines the message I'm prepared to make an exception (no pun intended).
---
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: Fix: various 20141028
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/incubator-brooklyn/pull/280
---
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: Fix: various 20141028
Posted by alasdairhodge <gi...@git.apache.org>.
Github user alasdairhodge commented on a diff in the pull request:
https://github.com/apache/incubator-brooklyn/pull/280#discussion_r19541090
--- Diff: software/base/src/main/java/brooklyn/entity/brooklynnode/BrooklynNodeImpl.java ---
@@ -251,10 +267,21 @@ public Void call(ConfigBag parameters) {
TaskTags.markInessential(shutdownTask);
}
DynamicTasks.queue(shutdownTask).asTask().getUnchecked();
+ waitForShutdown(Duration.ONE_MINUTE);
+
DynamicTasks.queue(Effectors.invocation(entity(), STOP, ConfigBag.EMPTY)).asTask().getUnchecked();
Entities.destroy(entity);
return null;
}
+
+ private void waitForShutdown(Duration timeout) {
+ Entity entity = entity();
+ Repeater.create()
+ .until(entity, EntityPredicates.attributeEqualTo(BrooklynNode.WEB_CONSOLE_ACCESSIBLE, false))
+ .backoffTo(Duration.FIVE_SECONDS)
+ .limitTimeTo(timeout)
+ .runRequiringTrue();
+ }
--- End diff --
Duplicated method; move to static method in outer `BrooklynNode` 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: Fix: various 20141028
Posted by alasdairhodge <gi...@git.apache.org>.
Github user alasdairhodge commented on a diff in the pull request:
https://github.com/apache/incubator-brooklyn/pull/280#discussion_r19541925
--- Diff: software/nosql/src/main/java/brooklyn/entity/nosql/elasticsearch/ElasticSearchNodeSshDriver.java ---
@@ -54,7 +54,7 @@ public void install() {
String saveAs = resolver.getFilename();
List<String> commands = ImmutableList.<String>builder()
- .add(BashCommands.installJavaLatestOrFail())
+ .add(BashCommands.installJavaLatestOrWarn())
--- End diff --
Uh... Why isn't this a show-stopper?
---
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: Fix: various 20141028
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/280#discussion_r19548357
--- Diff: software/nosql/src/main/java/brooklyn/entity/nosql/elasticsearch/ElasticSearchNodeSshDriver.java ---
@@ -54,7 +54,7 @@ public void install() {
String saveAs = resolver.getFilename();
List<String> commands = ImmutableList.<String>builder()
- .add(BashCommands.installJavaLatestOrFail())
+ .add(BashCommands.installJavaLatestOrWarn())
--- End diff --
On OS X, it doesn't automatically install Java so fails. But Java was already installed.
When I changed to `OrWarn` then I could then run the `ElasticSearchNodeSshDriver` integration tests on OS X.
Arguably we should do a better job of checking if Java is already installed!
Currently our `BashCommands.installJava(int version)` doesn't check the current version at all.
---
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: Fix: various 20141028
Posted by aledsage <gi...@git.apache.org>.
Github user aledsage commented on the pull request:
https://github.com/apache/incubator-brooklyn/pull/280#issuecomment-60951203
Thanks @alasdairhodge - I've incorporated those comments and rebased. Any further thoughts on use of `installJavaLatestOrWarn`? Is this ok to 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: Fix: various 20141028
Posted by alasdairhodge <gi...@git.apache.org>.
Github user alasdairhodge commented on a diff in the pull request:
https://github.com/apache/incubator-brooklyn/pull/280#discussion_r19541508
--- Diff: software/nosql/src/test/java/brooklyn/entity/nosql/elasticsearch/ElasticSearchClusterIntegrationTest.java ---
@@ -103,9 +109,20 @@ public void testPutAndGet() throws URISyntaxException {
new HttpGet(getBaseUri + "/mydocuments/docs/1/_source"));
assertEquals(getResponse.getResponseCode(), 200);
assertEquals(HttpValueFunctions.jsonContents("foo", String.class).apply(getResponse), "bar");
-
- totalDocumentCount += node.getAttribute(ElasticSearchNode.DOCUMENT_COUNT);
}
- assertEquals(totalDocumentCount, 1);
+ Asserts.succeedsEventually(new Runnable() {
+ public void run() {
+ int count = clusterDocumentCount();
+ assertTrue(count >= 1, "count="+count);
+ LOG.info("Document count is "+count);
--- End diff --
`debug` instead of `info`?
---
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: Fix: various 20141028
Posted by alasdairhodge <gi...@git.apache.org>.
Github user alasdairhodge commented on the pull request:
https://github.com/apache/incubator-brooklyn/pull/280#issuecomment-60953031
The java installation stuff is ropey, but should be addressed in a separate PR. This looks good to go.
---
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.
---