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 2015/03/06 13:41:17 UTC
[GitHub] incubator-brooklyn pull request: Mirroring entities should wait fo...
GitHub user neykov opened a pull request:
https://github.com/apache/incubator-brooklyn/pull/542
Mirroring entities should wait for a proper restart of remote
There is a window of time when restarting Brooklyn where it returns 404 for its entities, making mirroring entities unmanage themselves. Fix it by extending the existing filtering logic to return 403 for explicitly marked endpoints while remote is STANDBY or rebinding.
I had to use a delay after master promotion because there is still a small period of time just after the node becomes master but before it starts rebinding where it will return 404 for entities and there are no flags to check for it.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/neykov/incubator-brooklyn brooklyn-node-restart
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/incubator-brooklyn/pull/542.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 #542
----
commit bdd90d1f1e6e5573bad7bdb28d2f09438e98a2c3
Author: Svetoslav Neykov <sv...@cloudsoftcorp.com>
Date: 2015-02-27T10:52:29Z
Replace usage of STOP_MACHINE=true with STOP_MACHINE_MODE=IF_NOT_STOPPED
commit 332a7812ec6835cf3584dd0e3e03e1be17da8462
Author: Svetoslav Neykov <sv...@cloudsoftcorp.com>
Date: 2015-02-27T10:54:00Z
Add {pre,post}RestartCustom hooks
Can't rely on {pre,post}Stop/Start hooks being called on restart.
commit 668b88f1ec7ba2e8069968e7f9cfae4bef313a0c
Author: Svetoslav Neykov <sv...@cloudsoftcorp.com>
Date: 2015-02-27T10:54:21Z
Shutdown BrooklynNode gracefully on restart
commit beda5e62adb26f58e3e355bc230b65a508738296
Author: Svetoslav Neykov <sv...@cloudsoftcorp.com>
Date: 2015-02-27T11:19:48Z
Improve documentation for STOP effector parameters
commit 2ddecbe5bf06089536f459272c9863ad93d13949
Author: Svetoslav Neykov <sv...@cloudsoftcorp.com>
Date: 2015-02-27T15:08:35Z
Don't shutdown BrooklynNode if STOP_PROCESS_MODE says so.
commit 63af576254303ba792407c03bcc9aa6318ddcaa9
Author: Svetoslav Neykov <sv...@cloudsoftcorp.com>
Date: 2015-02-27T15:08:46Z
Umanage BrooklynNode only if machine is released.
Previous logic of looking at the effector paramaters wasn't working in all cases as IF_NOT_STOPPED depends on external factors. Instead check if the SshMachineLocation is still around.
commit 827a828d5bfd496a735e2fc7cb33181d5ca9783d
Author: Svetoslav Neykov <sv...@cloudsoftcorp.com>
Date: 2015-03-04T14:50:00Z
Decline requests for resources relying on rebinded state in non HOT states
No point in asking for applications when HA state is STANDBY. For example 404 response should mean that the resource doesn't exist, not that it's not loaded yet.
commit 1242a73560a05f55234e359afd16b60498071f74
Author: Svetoslav Neykov <sv...@cloudsoftcorp.com>
Date: 2015-03-04T15:05:56Z
Deny resource requests during rebind
commit 0483bdd997f0b767584cc81689413623e3198713
Author: Svetoslav Neykov <sv...@cloudsoftcorp.com>
Date: 2015-03-05T13:06:20Z
Wait for the HA state to stabilize before letting requests in
There is a period of time when the state is MASTER, but the context is not fully initialized.
commit d1880724cd806667355acb56917856be725e8ca9
Author: Svetoslav Neykov <sv...@cloudsoftcorp.com>
Date: 2015-03-05T13:07:53Z
Tests for the HA HOT state check for resource access
commit 0be15c5682b91ba7df23a9f1f136e76083bb07e8
Author: Svetoslav Neykov <sv...@cloudsoftcorp.com>
Date: 2015-03-05T13:11:07Z
Rename filter to follow naming convention
commit 09721506bdc7a0bb89a1e360c1b7ebb94cacfd44
Author: Svetoslav Neykov <sv...@cloudsoftcorp.com>
Date: 2015-03-06T12:02:31Z
Integration tests for resource accessibility with regard to the HA 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: Mirroring entities should wait fo...
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/542#discussion_r26207831
--- Diff: software/base/src/main/java/brooklyn/entity/brooklynnode/BrooklynNodeImpl.java ---
@@ -202,6 +229,15 @@ protected void postStop() {
}
}
+ private boolean isStopMachine() {
--- End diff --
I was thrown by this method name when I looked at the impl.
Should this method be called `isStoppingMachine()` or `isMachineStopped()` (I prefer the former)?
---
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: Mirroring entities should wait fo...
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/542#discussion_r26208106
--- Diff: usage/rest-server/src/test/java/brooklyn/rest/HaHotCheckTest.java ---
@@ -0,0 +1,142 @@
+/*
+ * 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.rest;
+
+import static org.testng.Assert.assertEquals;
+
+import javax.ws.rs.core.MediaType;
+
+import org.testng.annotations.AfterClass;
+import org.testng.annotations.AfterMethod;
+import org.testng.annotations.BeforeClass;
+import org.testng.annotations.BeforeMethod;
+import org.testng.annotations.Test;
+
+import brooklyn.management.ha.HighAvailabilityManager;
+import brooklyn.management.ha.ManagementNodeState;
+import brooklyn.rest.filter.HaHotCheckResourceFilter;
+import brooklyn.rest.filter.HaMasterCheckFilter;
+import brooklyn.rest.testing.BrooklynRestResourceTest;
+import brooklyn.rest.testing.mocks.ManagementContextMock;
+import brooklyn.rest.util.HaHotStateCheckClassResource;
+import brooklyn.rest.util.HaHotStateCheckResource;
+import brooklyn.rest.util.ManagementContextProvider;
+
+import com.sun.jersey.api.client.ClientResponse;
+import com.sun.jersey.api.client.WebResource.Builder;
+import com.sun.jersey.api.core.ResourceConfig;
+
+public class HaHotCheckTest extends BrooklynRestResourceTest {
+
+ private ManagementContextMock mgmtMock;
+
+ //Treat before/after class methods as before/after method
+ //otherwise they should be static.
--- End diff --
I don't follow. The same instance is used for all the test methods, so `@BeforeClass` should not be static.
---
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: Mirroring entities should wait fo...
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/542#discussion_r26208010
--- Diff: usage/rest-server/src/main/java/brooklyn/rest/filter/HaHotStateRequired.java ---
@@ -0,0 +1,28 @@
+/*
+ * 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.rest.filter;
+
+import java.lang.annotation.ElementType;
+import java.lang.annotation.Retention;
+import java.lang.annotation.RetentionPolicy;
+import java.lang.annotation.Target;
+
+@Retention(RetentionPolicy.RUNTIME)
+@Target({ElementType.METHOD, ElementType.TYPE})
+public @interface HaHotStateRequired {}
--- End diff --
worth javadoc saying what annotation means, and where it should be used
---
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: Mirroring entities should wait fo...
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/542#discussion_r26207634
--- Diff: core/src/main/java/brooklyn/management/ha/HighAvailabilityManagerImpl.java ---
@@ -1077,4 +1077,13 @@ public String toString() {
return result;
}
+ @Override
+ public long getLastStateChange() {
+ if (nodeStateHistory.size() > 0) {
+ return (Long)nodeStateHistory.get(0).get("timestamp");
+ } else {
+ return 0;
--- End diff --
Personal preference for returning `-1` to signify not available (as 0 is an unlikely but valid timestamp)
---
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: Mirroring entities should wait fo...
Posted by ahgittin <gi...@git.apache.org>.
Github user ahgittin commented on the pull request:
https://github.com/apache/incubator-brooklyn/pull/542#issuecomment-84006625
just came across the rest filter -- very nice piece of work. it breaks some things in the gui when connecting to a standby server, and i wondered why. not a big problem, and probably wiser to fix that in the JS side, keeping the filter changes here, but wanted to note that.
also it might be nice to merge `HaHotCheckResourceFilter` with the `HaMasterCheckFilter` but again minor. i've put TODO's in the code in PR coming 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: Mirroring entities should wait fo...
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/542#discussion_r26208033
--- Diff: usage/rest-server/src/main/java/brooklyn/rest/resources/ApplicationResource.java ---
@@ -82,6 +83,7 @@
import com.google.common.collect.FluentIterable;
import com.google.common.collect.Iterables;
+@HaHotStateRequired
--- End diff --
Is it right to have these annotations on the Resource rather than the API interface? No strong feelings.
---
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: Mirroring entities should wait fo...
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/542#discussion_r26207663
--- Diff: software/base/src/main/java/brooklyn/entity/brooklynnode/BrooklynNodeImpl.java ---
@@ -153,17 +158,41 @@ protected void preStopConfirmCustom() {
@Override
protected void preStop() {
super.preStop();
+ if (MachineLifecycleEffectorTasks.canStop(getStopProcessModeParam(), this)) {
+ shutdownGracefully();
+ }
+ }
+
+ private StopMode getStopProcessModeParam() {
+ ConfigBag parameters = BrooklynTaskTags.getCurrentEffectorParameters();
+ if (parameters != null) {
+ return parameters.get(StopSoftwareParameters.STOP_PROCESS_MODE);
--- End diff --
also need to consider the deprecated `STOP_MACHINE` parameter option?
---
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: Mirroring entities should wait fo...
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/542#discussion_r26615641
--- Diff: usage/rest-server/src/main/java/brooklyn/rest/resources/ApplicationResource.java ---
@@ -82,6 +83,7 @@
import com.google.common.collect.FluentIterable;
import com.google.common.collect.Iterables;
+@HaHotStateRequired
--- End diff --
Both ways can be used, but having class-wide annotation is better in this case as it is too easy to miss the annotation in newly added methods. If we need to be more selective, then method-scope is implemented as well.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-brooklyn pull request: Mirroring entities should wait fo...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/incubator-brooklyn/pull/542
---
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: Mirroring entities should wait fo...
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/542#discussion_r26207980
--- Diff: usage/jsgui/src/test/java/brooklyn/rest/jsgui/BrooklynJavascriptGuiLauncherTest.java ---
@@ -60,8 +61,15 @@ public void testJavascriptWithRest() throws Exception {
checkUrlContains("/v1/catalog/entities", "Tomcat");
}
- protected void checkUrlContains(String path, String text) {
- HttpTestUtils.assertContentContainsText(rootUrl()+path, text);
+ protected void checkUrlContains(final String path, final String text) {
+ //Server may return 403 until it loads completely, wait a bit
+ //until it stabilizes.
+ Asserts.succeedsEventually(new Runnable() {
--- End diff --
Can use `HttpTestUtils.assertContentEventuallyContainsText(rootUrl()+path, text)`.
I'd really like consistency for our assertXxxEventuallyYyy versus assertXxxYyyEventually. Personally, I much prefer the latter because we often also have an assertXxxYyy method - having the Eventually as a suffix keeps the methods together more. But that's an aside...
---
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: Mirroring entities should wait fo...
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/542#discussion_r26207687
--- Diff: software/base/src/main/java/brooklyn/entity/brooklynnode/BrooklynNodeImpl.java ---
@@ -153,17 +158,41 @@ protected void preStopConfirmCustom() {
@Override
protected void preStop() {
super.preStop();
+ if (MachineLifecycleEffectorTasks.canStop(getStopProcessModeParam(), this)) {
+ shutdownGracefully();
+ }
+ }
+
+ private StopMode getStopProcessModeParam() {
+ ConfigBag parameters = BrooklynTaskTags.getCurrentEffectorParameters();
+ if (parameters != null) {
+ return parameters.get(StopSoftwareParameters.STOP_PROCESS_MODE);
+ } else {
+ return StopSoftwareParameters.STOP_PROCESS_MODE.getDefaultValue();
+ }
+ }
+ @Override
+ protected void preRestart() {
+ super.preRestart();
+ shutdownGracefully();
+ DynamicTasks.queue("post-shutdown", new Runnable() { public void run() {
--- End diff --
Should be `"pre-restart`" presumably?
---
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: Mirroring entities should wait fo...
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/542#discussion_r26208116
--- Diff: usage/rest-server/src/test/java/brooklyn/rest/HaHotCheckTest.java ---
@@ -0,0 +1,142 @@
+/*
+ * 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.rest;
+
+import static org.testng.Assert.assertEquals;
+
+import javax.ws.rs.core.MediaType;
+
+import org.testng.annotations.AfterClass;
+import org.testng.annotations.AfterMethod;
+import org.testng.annotations.BeforeClass;
+import org.testng.annotations.BeforeMethod;
+import org.testng.annotations.Test;
+
+import brooklyn.management.ha.HighAvailabilityManager;
+import brooklyn.management.ha.ManagementNodeState;
+import brooklyn.rest.filter.HaHotCheckResourceFilter;
+import brooklyn.rest.filter.HaMasterCheckFilter;
+import brooklyn.rest.testing.BrooklynRestResourceTest;
+import brooklyn.rest.testing.mocks.ManagementContextMock;
+import brooklyn.rest.util.HaHotStateCheckClassResource;
+import brooklyn.rest.util.HaHotStateCheckResource;
+import brooklyn.rest.util.ManagementContextProvider;
+
+import com.sun.jersey.api.client.ClientResponse;
+import com.sun.jersey.api.client.WebResource.Builder;
+import com.sun.jersey.api.core.ResourceConfig;
+
+public class HaHotCheckTest extends BrooklynRestResourceTest {
+
+ private ManagementContextMock mgmtMock;
+
+ //Treat before/after class methods as before/after method
+ //otherwise they should be static.
+ @BeforeClass(alwaysRun=true)
+ public void setUpClass() {}
--- End diff --
It's a bit confusing overriding the setUp and changing it from `@BeforeClass` in super to `@BeforeMethod`.
---
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: Mirroring entities should wait fo...
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/542#discussion_r26207850
--- Diff: software/base/src/test/java/brooklyn/entity/software/MachineLifecycleEffectorTasksTest.java ---
@@ -23,11 +23,17 @@
import org.testng.annotations.DataProvider;
import org.testng.annotations.Test;
+import brooklyn.entity.basic.BasicEntityImpl;
+import brooklyn.entity.basic.Lifecycle;
+import brooklyn.entity.basic.SoftwareProcess;
import brooklyn.entity.basic.SoftwareProcess.StopSoftwareParameters.StopMode;
public class MachineLifecycleEffectorTasksTest {
public static boolean canStop(StopMode stopMode, boolean isEntityStopped) {
- return MachineLifecycleEffectorTasks.canStop(stopMode, isEntityStopped);
+ BasicEntityImpl entity = new BasicEntityImpl();
--- End diff --
ok for now, but at some point calling the entity constructor directly may give us very surprising subsequent behaviour (e.g. because not managed, and no management context injected).
---
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: Mirroring entities should wait fo...
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/542#discussion_r26616906
--- Diff: usage/rest-server/src/test/java/brooklyn/rest/BrooklynRestApiLauncher.java ---
@@ -150,6 +152,11 @@ public BrooklynRestApiLauncher withoutJsgui() {
this.deployJsgui = false;
return this;
}
+
+ public BrooklynRestApiLauncher disableHighAvailability(boolean value) {
--- End diff --
I agree with you, I also prefer positive naming. In this case though it's more specific. When the flag is `disableHighAvailability=true` the code calls
```
mgmt.getHighAvailabilityManager().disabled();
```
Having it named `enableHighAvailability=true` I would expect that it calls start on the HA. By adding the flag I wanted to be able to say "don't disable HA".
So wdyt, I don't mind changing it, but `enableHighAvailability` could lead to unmet expectations?
---
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: Mirroring entities should wait fo...
Posted by neykov <gi...@git.apache.org>.
Github user neykov commented on the pull request:
https://github.com/apache/incubator-brooklyn/pull/542#issuecomment-82618996
Thanks for the great review @aledsage. Addressed comments and added your test case.
Merge conflicts need to be resolved due to https://github.com/apache/incubator-brooklyn/pull/553, needs some extra attention.
---
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: Mirroring entities should wait fo...
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/542#discussion_r26656373
--- Diff: usage/rest-server/src/test/java/brooklyn/rest/BrooklynRestApiLauncher.java ---
@@ -150,6 +152,11 @@ public BrooklynRestApiLauncher withoutJsgui() {
this.deployJsgui = false;
return this;
}
+
+ public BrooklynRestApiLauncher disableHighAvailability(boolean value) {
--- End diff --
Makes sense; agreed.
---
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: Mirroring entities should wait fo...
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/542#discussion_r26208192
--- Diff: usage/rest-server/src/test/java/brooklyn/rest/testing/mocks/HighAvailabilityManagerMock.java ---
@@ -0,0 +1,121 @@
+/*
+ * 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.rest.testing.mocks;
+
+import java.util.Map;
+
+import brooklyn.management.ha.HighAvailabilityManager;
+import brooklyn.management.ha.HighAvailabilityMode;
+import brooklyn.management.ha.ManagementNodeState;
+import brooklyn.management.ha.ManagementPlaneSyncRecord;
+import brooklyn.management.ha.ManagementPlaneSyncRecordPersister;
+
+public class HighAvailabilityManagerMock implements HighAvailabilityManager {
--- End diff --
Strictly speaking, this is a stub rather than a mock (http://martinfowler.com/articles/mocksArentStubs.html).
Personal preference for the class name to have `Stub` suffix 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: Mirroring entities should wait fo...
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/542#discussion_r26613978
--- Diff: software/base/src/main/java/brooklyn/entity/brooklynnode/BrooklynNodeImpl.java ---
@@ -153,17 +158,41 @@ protected void preStopConfirmCustom() {
@Override
protected void preStop() {
super.preStop();
+ if (MachineLifecycleEffectorTasks.canStop(getStopProcessModeParam(), this)) {
+ shutdownGracefully();
+ }
+ }
+
+ private StopMode getStopProcessModeParam() {
+ ConfigBag parameters = BrooklynTaskTags.getCurrentEffectorParameters();
+ if (parameters != null) {
+ return parameters.get(StopSoftwareParameters.STOP_PROCESS_MODE);
--- End diff --
This concerns the process only, stop machine mode not relevant.
---
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: Mirroring entities should wait fo...
Posted by aledsage <gi...@git.apache.org>.
Github user aledsage commented on the pull request:
https://github.com/apache/incubator-brooklyn/pull/542#issuecomment-78255699
Looks really good! A few minor comments to be addressed, but very close to being merged.
FYI I did a tiny bit of this in https://github.com/apache/incubator-brooklyn/pull/547 (see commits https://github.com/aledsage/incubator-brooklyn/commit/fe5ecca123525b4d6c8cd9f5c0049e3d5b6b12b6 "BrooklynNode: only unmanage if VM terminating", https://github.com/aledsage/incubator-brooklyn/commit/f736dfc84f79d3aa6275747ad3bdf6e4d9181a0d "Add SoftwareProcess.preRestart (for BrooklynNode)", and https://github.com/aledsage/incubator-brooklyn/commit/d38b652519cf4b46632c1b8dd06c52498eb3cdd3 "BrooklynCluster: add memberSpec + test").
I prefer your impl of `preRestartCustom` (that you included the method on the superclass as well, and that you also added `postRestart` for symmetry.
The test I added is useful though. I can look at that again once this is merged and I rebase my PR, or you can pull in my test if you fancy.
---
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: Mirroring entities should wait fo...
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/542#discussion_r26207695
--- Diff: software/base/src/main/java/brooklyn/entity/brooklynnode/BrooklynNodeImpl.java ---
@@ -153,17 +158,41 @@ protected void preStopConfirmCustom() {
@Override
protected void preStop() {
super.preStop();
+ if (MachineLifecycleEffectorTasks.canStop(getStopProcessModeParam(), this)) {
+ shutdownGracefully();
+ }
+ }
+
+ private StopMode getStopProcessModeParam() {
+ ConfigBag parameters = BrooklynTaskTags.getCurrentEffectorParameters();
+ if (parameters != null) {
+ return parameters.get(StopSoftwareParameters.STOP_PROCESS_MODE);
+ } else {
+ return StopSoftwareParameters.STOP_PROCESS_MODE.getDefaultValue();
+ }
+ }
+ @Override
+ protected void preRestart() {
+ super.preRestart();
+ shutdownGracefully();
--- End diff --
Deserves a comment to say why doing shutdown here
---
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: Mirroring entities should wait fo...
Posted by aledsage <gi...@git.apache.org>.
Github user aledsage commented on the pull request:
https://github.com/apache/incubator-brooklyn/pull/542#issuecomment-82930061
@neykov once you've resolved merge conflicts, this will be ready 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: Mirroring entities should wait fo...
Posted by aledsage <gi...@git.apache.org>.
Github user aledsage commented on the pull request:
https://github.com/apache/incubator-brooklyn/pull/542#issuecomment-83591861
Thanks; merging now.
---
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: Mirroring entities should wait fo...
Posted by neykov <gi...@git.apache.org>.
Github user neykov commented on the pull request:
https://github.com/apache/incubator-brooklyn/pull/542#issuecomment-82996126
Rebased, merge conflicts resolved.
---
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: Mirroring entities should wait fo...
Posted by neykov <gi...@git.apache.org>.
Github user neykov commented on the pull request:
https://github.com/apache/incubator-brooklyn/pull/542#issuecomment-83591468
Rebased, merge conflicts resolved.
---
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: Mirroring entities should wait fo...
Posted by ahgittin <gi...@git.apache.org>.
Github user ahgittin commented on the pull request:
https://github.com/apache/incubator-brooklyn/pull/542#issuecomment-78884808
agree, changes here look really good, and would be nice to pull in @aledsage 's addl test. /cc @neykov
---
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: Mirroring entities should wait fo...
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/542#discussion_r26208066
--- Diff: usage/rest-server/src/test/java/brooklyn/rest/BrooklynRestApiLauncher.java ---
@@ -150,6 +152,11 @@ public BrooklynRestApiLauncher withoutJsgui() {
this.deployJsgui = false;
return this;
}
+
+ public BrooklynRestApiLauncher disableHighAvailability(boolean value) {
--- End diff --
Personal preference for fields/methods to be written in the positive rather than the negative.
We could call the method/field `enableHighAvailability(boolean)`, defaulting to true.
Other such examples include:
* `JcloudsLocation.USE_JCLOUDS_SSH_INIT` rather than `DO_NOT_USE_JCLOUDS_SSH_INIT`.
* `org.testng.annotations.Test.enabled` (rather than disabled)
---
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.
---