You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@brooklyn.apache.org by ahgittin <gi...@git.apache.org> on 2017/09/13 16:54:46 UTC
[GitHub] brooklyn-library pull request #126: Highlights and load
GitHub user ahgittin opened a pull request:
https://github.com/apache/brooklyn-library/pull/126
Highlights and load
uses https://github.com/apache/brooklyn-server/pull/818 to add highlights for software-specific entities. also adds new perf test.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/ahgittin/brooklyn-library highlights-and-load
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/brooklyn-library/pull/126.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 #126
----
commit 71a3fd2ebd2652dce2fd2b9678bffaed613eec27
Author: Alex Heneveld <al...@cloudsoftcorp.com>
Date: 2017-09-13T16:52:08Z
add load test without persistence
so we can see what else is chewing up cpu
commit 4babb503460bbd938352b603c397214e53a1ef08
Author: Alex Heneveld <al...@cloudsoftcorp.com>
Date: 2017-09-13T16:52:43Z
set highlights on software-specific enrichers and policies
----
---
[GitHub] brooklyn-library pull request #126: Highlights and load
Posted by tbouron <gi...@git.apache.org>.
Github user tbouron commented on a diff in the pull request:
https://github.com/apache/brooklyn-library/pull/126#discussion_r142681908
--- Diff: software/webapp/src/main/java/org/apache/brooklyn/entity/proxy/AbstractNonProvisionedControllerImpl.java ---
@@ -216,6 +218,7 @@ protected void removeServerPoolMemberTrackingPolicy() {
public static class ServerPoolMemberTrackerPolicy extends AbstractMembershipTrackingPolicy {
@Override
protected void onEntityEvent(EventType type, Entity entity) {
+ defaultHighlightAction(type, entity);
--- End diff --
Same as above
---
[GitHub] brooklyn-library issue #126: Highlights and load
Posted by ahgittin <gi...@git.apache.org>.
Github user ahgittin commented on the issue:
https://github.com/apache/brooklyn-library/pull/126
hmm, failure is related:
```
[ERROR] /home/jenkins/jenkins-slave/workspace/brooklyn-library-pull-requests/software/network/src/test/java/org/apache/brooklyn/entity/network/bind/PrefixAndIdEnricher.java:[56,9] cannot find symbol
symbol: method highlightTriggers(org.apache.brooklyn.api.sensor.AttributeSensor<capture#1 of ?>,<nulltype>)
location: class org.apache.brooklyn.entity.network.bind.PrefixAndIdEnricher
```
it's trying to reference `org.apache.brooklyn:brooklyn-parent:0.12.0-SNAPSHOT`. have merged in master branch which changes deps to 0.13.0, retest will now hopefully be happy.
---
[GitHub] brooklyn-library pull request #126: Highlights and load
Posted by tbouron <gi...@git.apache.org>.
Github user tbouron commented on a diff in the pull request:
https://github.com/apache/brooklyn-library/pull/126#discussion_r142680872
--- Diff: software/nosql/src/main/java/org/apache/brooklyn/entity/nosql/mongodb/sharding/MongoDBRouterClusterImpl.java ---
@@ -56,12 +60,17 @@ public void start(Collection<? extends Location> locations) {
public static class MemberTrackingPolicy extends AbstractMembershipTrackingPolicy {
@Override protected void onEntityEvent(EventType type, Entity member) {
+ defaultHighlightAction(type, entity);
((MongoDBRouterClusterImpl)super.entity).setAnyRouter();
}
@Override protected void onEntityRemoved(Entity member) {
+ // TODO shouldn't be invoked - remove
+ log.warn("Removal handler should be hidden by event handler", new Throwable("Trace for unexpected mongo node handler"));
((MongoDBRouterClusterImpl)super.entity).setAnyRouter();
}
@Override protected void onEntityChange(Entity member) {
+ // TODO shouldn't be invoked - remove
+ log.warn("Change handler should be hidden by event handler", new Throwable("Trace for unexpected mongo node handler"));
--- End diff --
Same as above
---
[GitHub] brooklyn-library pull request #126: Highlights and load
Posted by ahgittin <gi...@git.apache.org>.
Github user ahgittin commented on a diff in the pull request:
https://github.com/apache/brooklyn-library/pull/126#discussion_r142703228
--- Diff: software/nosql/src/main/java/org/apache/brooklyn/entity/nosql/mongodb/sharding/MongoDBRouterClusterImpl.java ---
@@ -56,12 +60,17 @@ public void start(Collection<? extends Location> locations) {
public static class MemberTrackingPolicy extends AbstractMembershipTrackingPolicy {
@Override protected void onEntityEvent(EventType type, Entity member) {
+ defaultHighlightAction(type, entity);
--- End diff --
this method overrides that method and doesn't call `super` so it has to redo it here, doesn't it?
---
[GitHub] brooklyn-library pull request #126: Highlights and load
Posted by tbouron <gi...@git.apache.org>.
Github user tbouron commented on a diff in the pull request:
https://github.com/apache/brooklyn-library/pull/126#discussion_r142717962
--- Diff: software/nosql/src/main/java/org/apache/brooklyn/entity/nosql/mongodb/sharding/MongoDBRouterClusterImpl.java ---
@@ -56,12 +60,17 @@ public void start(Collection<? extends Location> locations) {
public static class MemberTrackingPolicy extends AbstractMembershipTrackingPolicy {
@Override protected void onEntityEvent(EventType type, Entity member) {
+ defaultHighlightAction(type, entity);
((MongoDBRouterClusterImpl)super.entity).setAnyRouter();
}
@Override protected void onEntityRemoved(Entity member) {
+ // TODO shouldn't be invoked - remove
+ log.warn("Removal handler should be hidden by event handler", new Throwable("Trace for unexpected mongo node handler"));
--- End diff --
That make sense now, as `onEntityEvent` does not call `super`
---
[GitHub] brooklyn-library pull request #126: Highlights and load
Posted by tbouron <gi...@git.apache.org>.
Github user tbouron commented on a diff in the pull request:
https://github.com/apache/brooklyn-library/pull/126#discussion_r142718981
--- Diff: qa/src/test/java/org/apache/brooklyn/qa/load/AbstractLoadTest.java ---
@@ -161,8 +161,8 @@ protected ManagementContext setUpPlatform() {
// Create management node
persistenceDir = Files.createTempDir();
launcher = BrooklynLauncher.newInstance()
- .persistMode(PersistMode.CLEAN)
- .highAvailabilityMode(HighAvailabilityMode.MASTER)
+ .persistMode(doPersistence() ? PersistMode.CLEAN : PersistMode.DISABLED)
+ .highAvailabilityMode(doPersistence() ? HighAvailabilityMode.MASTER : HighAvailabilityMode.DISABLED)
--- End diff --
Good point, didn't think of the complexity due to the framework and annotation. Your solution works so let's go with that, we can iterate on it later on
---
[GitHub] brooklyn-library pull request #126: Highlights and load
Posted by ahgittin <gi...@git.apache.org>.
Github user ahgittin commented on a diff in the pull request:
https://github.com/apache/brooklyn-library/pull/126#discussion_r142702860
--- Diff: qa/src/test/java/org/apache/brooklyn/qa/load/AbstractLoadTest.java ---
@@ -161,8 +161,8 @@ protected ManagementContext setUpPlatform() {
// Create management node
persistenceDir = Files.createTempDir();
launcher = BrooklynLauncher.newInstance()
- .persistMode(PersistMode.CLEAN)
- .highAvailabilityMode(HighAvailabilityMode.MASTER)
+ .persistMode(doPersistence() ? PersistMode.CLEAN : PersistMode.DISABLED)
+ .highAvailabilityMode(doPersistence() ? HighAvailabilityMode.MASTER : HighAvailabilityMode.DISABLED)
--- End diff --
due to how tests are structured and annotated methods run by the framework, methods for this sort of thing are easier to work with i find. happy if you want to spike an alternative with fields but i doubt it's better.
---
[GitHub] brooklyn-library pull request #126: Highlights and load
Posted by ahgittin <gi...@git.apache.org>.
Github user ahgittin commented on a diff in the pull request:
https://github.com/apache/brooklyn-library/pull/126#discussion_r142703444
--- Diff: software/nosql/src/main/java/org/apache/brooklyn/entity/nosql/mongodb/sharding/MongoDBRouterClusterImpl.java ---
@@ -56,12 +60,17 @@ public void start(Collection<? extends Location> locations) {
public static class MemberTrackingPolicy extends AbstractMembershipTrackingPolicy {
@Override protected void onEntityEvent(EventType type, Entity member) {
+ defaultHighlightAction(type, entity);
((MongoDBRouterClusterImpl)super.entity).setAnyRouter();
}
@Override protected void onEntityRemoved(Entity member) {
+ // TODO shouldn't be invoked - remove
+ log.warn("Removal handler should be hidden by event handler", new Throwable("Trace for unexpected mongo node handler"));
--- End diff --
more a note to developer (it caught me out) - due to the `onEntityEvent` handler being overridden this method is not used
---
[GitHub] brooklyn-library pull request #126: Highlights and load
Posted by tbouron <gi...@git.apache.org>.
Github user tbouron commented on a diff in the pull request:
https://github.com/apache/brooklyn-library/pull/126#discussion_r142681823
--- Diff: software/webapp/src/main/java/org/apache/brooklyn/entity/proxy/AbstractNonProvisionedControllerImpl.java ---
@@ -66,6 +67,7 @@ public AbstractNonProvisionedControllerImpl() {
public static class MemberTrackingPolicy extends AbstractMembershipTrackingPolicy {
@Override protected void onEntityEvent(EventType type, Entity member) {
+ defaultHighlightAction(type, entity);
--- End diff --
Same as above
---
[GitHub] brooklyn-library pull request #126: Highlights and load
Posted by tbouron <gi...@git.apache.org>.
Github user tbouron commented on a diff in the pull request:
https://github.com/apache/brooklyn-library/pull/126#discussion_r142680516
--- Diff: software/nosql/src/main/java/org/apache/brooklyn/entity/nosql/mongodb/sharding/MongoDBRouterClusterImpl.java ---
@@ -56,12 +60,17 @@ public void start(Collection<? extends Location> locations) {
public static class MemberTrackingPolicy extends AbstractMembershipTrackingPolicy {
@Override protected void onEntityEvent(EventType type, Entity member) {
+ defaultHighlightAction(type, entity);
--- End diff --
Not needed as this policy extends `AbstractMembershipTrackingPolicy ` and `onEntityEvent` [already call `defaultHighlightAction`](https://github.com/apache/brooklyn-server/blob/master/core/src/main/java/org/apache/brooklyn/entity/group/AbstractMembershipTrackingPolicy.java#L242)
---
[GitHub] brooklyn-library pull request #126: Highlights and load
Posted by tbouron <gi...@git.apache.org>.
Github user tbouron commented on a diff in the pull request:
https://github.com/apache/brooklyn-library/pull/126#discussion_r142680824
--- Diff: software/nosql/src/main/java/org/apache/brooklyn/entity/nosql/mongodb/sharding/MongoDBRouterClusterImpl.java ---
@@ -56,12 +60,17 @@ public void start(Collection<? extends Location> locations) {
public static class MemberTrackingPolicy extends AbstractMembershipTrackingPolicy {
@Override protected void onEntityEvent(EventType type, Entity member) {
+ defaultHighlightAction(type, entity);
((MongoDBRouterClusterImpl)super.entity).setAnyRouter();
}
@Override protected void onEntityRemoved(Entity member) {
+ // TODO shouldn't be invoked - remove
+ log.warn("Removal handler should be hidden by event handler", new Throwable("Trace for unexpected mongo node handler"));
--- End diff --
Not sure I understand what this is for: is this an error per se? Looks like a removal is handled correctly so why the warn message? Can you give a bit more details ?
---
[GitHub] brooklyn-library pull request #126: Highlights and load
Posted by tbouron <gi...@git.apache.org>.
Github user tbouron commented on a diff in the pull request:
https://github.com/apache/brooklyn-library/pull/126#discussion_r142683487
--- Diff: qa/src/test/java/org/apache/brooklyn/qa/load/AbstractLoadTest.java ---
@@ -161,8 +161,8 @@ protected ManagementContext setUpPlatform() {
// Create management node
persistenceDir = Files.createTempDir();
launcher = BrooklynLauncher.newInstance()
- .persistMode(PersistMode.CLEAN)
- .highAvailabilityMode(HighAvailabilityMode.MASTER)
+ .persistMode(doPersistence() ? PersistMode.CLEAN : PersistMode.DISABLED)
+ .highAvailabilityMode(doPersistence() ? HighAvailabilityMode.MASTER : HighAvailabilityMode.DISABLED)
--- End diff --
I think for a flag, a field would be cleaner than a method to override
---
[GitHub] brooklyn-library issue #126: Highlights and load
Posted by ahgittin <gi...@git.apache.org>.
Github user ahgittin commented on the issue:
https://github.com/apache/brooklyn-library/pull/126
thanks for the comments @tbouron - i've replied, and think the PR as it stands is correct. wdyt?
---
[GitHub] brooklyn-library pull request #126: Highlights and load
Posted by tbouron <gi...@git.apache.org>.
Github user tbouron commented on a diff in the pull request:
https://github.com/apache/brooklyn-library/pull/126#discussion_r142717711
--- Diff: software/nosql/src/main/java/org/apache/brooklyn/entity/nosql/mongodb/sharding/MongoDBRouterClusterImpl.java ---
@@ -56,12 +60,17 @@ public void start(Collection<? extends Location> locations) {
public static class MemberTrackingPolicy extends AbstractMembershipTrackingPolicy {
@Override protected void onEntityEvent(EventType type, Entity member) {
+ defaultHighlightAction(type, entity);
--- End diff --
Meh, you're right, I didn't see the missing `super` call for some reason.
---
[GitHub] brooklyn-library issue #126: Highlights and load
Posted by ahgittin <gi...@git.apache.org>.
Github user ahgittin commented on the issue:
https://github.com/apache/brooklyn-library/pull/126
Retest this please.
---
[GitHub] brooklyn-library issue #126: Highlights and load
Posted by ahgittin <gi...@git.apache.org>.
Github user ahgittin commented on the issue:
https://github.com/apache/brooklyn-library/pull/126
tests good after merging master, so merging this
---
[GitHub] brooklyn-library issue #126: Highlights and load
Posted by tbouron <gi...@git.apache.org>.
Github user tbouron commented on the issue:
https://github.com/apache/brooklyn-library/pull/126
@ahgittin Yup, looks good but I would like Jenkins to be happy before merging it.
Looking at the logs, looks like the failure is unrelated though
---
[GitHub] brooklyn-library pull request #126: Highlights and load
Posted by tbouron <gi...@git.apache.org>.
Github user tbouron commented on a diff in the pull request:
https://github.com/apache/brooklyn-library/pull/126#discussion_r142681757
--- Diff: software/webapp/src/main/java/org/apache/brooklyn/entity/proxy/AbstractControllerImpl.java ---
@@ -154,6 +154,7 @@ protected void removeServerPoolMemberTrackingPolicy() {
public static class ServerPoolMemberTrackerPolicy extends AbstractMembershipTrackingPolicy {
@Override
protected void onEntityEvent(EventType type, Entity entity) {
+ defaultHighlightAction(type, entity);
--- End diff --
Same as above
---
[GitHub] brooklyn-library pull request #126: Highlights and load
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/brooklyn-library/pull/126
---
[GitHub] brooklyn-library pull request #126: Highlights and load
Posted by tbouron <gi...@git.apache.org>.
Github user tbouron commented on a diff in the pull request:
https://github.com/apache/brooklyn-library/pull/126#discussion_r142681857
--- Diff: software/webapp/src/main/java/org/apache/brooklyn/entity/proxy/nginx/NginxControllerImpl.java ---
@@ -178,6 +178,7 @@ protected void removeUrlMappingsMemberTrackerPolicy() {
public static class UrlMappingsMemberTrackerPolicy extends AbstractMembershipTrackingPolicy {
@Override
protected void onEntityEvent(EventType type, Entity entity) {
+ defaultHighlightAction(type, entity);
--- End diff --
Same as above
---
[GitHub] brooklyn-library pull request #126: Highlights and load
Posted by tbouron <gi...@git.apache.org>.
Github user tbouron commented on a diff in the pull request:
https://github.com/apache/brooklyn-library/pull/126#discussion_r142681706
--- Diff: software/webapp/src/main/java/org/apache/brooklyn/entity/dns/AbstractGeoDnsServiceImpl.java ---
@@ -180,6 +180,7 @@ protected void endTracker() {
public static class MemberTrackingPolicy extends AbstractMembershipTrackingPolicy {
@Override
protected void onEntityEvent(EventType type, Entity entity) {
+ defaultHighlightAction(type, entity);
--- End diff --
Not needed as this policy extends `AbstractMembershipTrackingPolicy ` and `onEntityEvent` [already call `defaultHighlightAction`](https://github.com/apache/brooklyn-server/blob/master/core/src/main/java/org/apache/brooklyn/entity/group/AbstractMembershipTrackingPolicy.java#L242)
---