Highlights and load by ahgittin · Pull Request #126 · apache/brooklyn-library

ahgittin · 2017-09-13T16:54:45Z

uses apache/brooklyn-server#818 to add highlights for software-specific entities. also adds new perf test.

so we can see what else is chewing up cpu

tbouron

Thanks @ahgittin, in general LGTM but I added a few comments.

tbouron · 2017-10-04T14:08:48Z

...rc/main/java/org/apache/brooklyn/entity/nosql/mongodb/sharding/MongoDBRouterClusterImpl.java


    public static class MemberTrackingPolicy extends AbstractMembershipTrackingPolicy {
        @Override protected void onEntityEvent(EventType type, Entity member) {
+            defaultHighlightAction(type, entity);


Not needed as this policy extends AbstractMembershipTrackingPolicy and onEntityEvent already call defaultHighlightAction

this method overrides that method and doesn't call super so it has to redo it here, doesn't it?

Meh, you're right, I didn't see the missing super call for some reason.

tbouron · 2017-10-04T14:09:53Z

...rc/main/java/org/apache/brooklyn/entity/nosql/mongodb/sharding/MongoDBRouterClusterImpl.java

        }
        @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"));


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 ?

more a note to developer (it caught me out) - due to the onEntityEvent handler being overridden this method is not used

That make sense now, as onEntityEvent does not call super

tbouron · 2017-10-04T14:10:01Z

...rc/main/java/org/apache/brooklyn/entity/nosql/mongodb/sharding/MongoDBRouterClusterImpl.java

        }
        @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"));


Same as above

tbouron · 2017-10-04T14:12:35Z

software/webapp/src/main/java/org/apache/brooklyn/entity/dns/AbstractGeoDnsServiceImpl.java

    public static class MemberTrackingPolicy extends AbstractMembershipTrackingPolicy {
        @Override
        protected void onEntityEvent(EventType type, Entity entity) {
+            defaultHighlightAction(type, entity);


Not needed as this policy extends AbstractMembershipTrackingPolicy and onEntityEvent already call defaultHighlightAction

tbouron · 2017-10-04T14:12:42Z

software/webapp/src/main/java/org/apache/brooklyn/entity/proxy/AbstractControllerImpl.java

    public static class ServerPoolMemberTrackerPolicy extends AbstractMembershipTrackingPolicy {
        @Override
        protected void onEntityEvent(EventType type, Entity entity) {
+            defaultHighlightAction(type, entity);


Same as above

tbouron · 2017-10-04T14:12:57Z

...app/src/main/java/org/apache/brooklyn/entity/proxy/AbstractNonProvisionedControllerImpl.java


    public static class MemberTrackingPolicy extends AbstractMembershipTrackingPolicy {
        @Override protected void onEntityEvent(EventType type, Entity member) {
+            defaultHighlightAction(type, entity);


Same as above

tbouron · 2017-10-04T14:13:03Z

software/webapp/src/main/java/org/apache/brooklyn/entity/proxy/nginx/NginxControllerImpl.java

    public static class UrlMappingsMemberTrackerPolicy extends AbstractMembershipTrackingPolicy {
        @Override
        protected void onEntityEvent(EventType type, Entity entity) {
+            defaultHighlightAction(type, entity);


Same as above

tbouron · 2017-10-04T14:13:13Z

...app/src/main/java/org/apache/brooklyn/entity/proxy/AbstractNonProvisionedControllerImpl.java

    public static class ServerPoolMemberTrackerPolicy extends AbstractMembershipTrackingPolicy {
        @Override
        protected void onEntityEvent(EventType type, Entity entity) {
+            defaultHighlightAction(type, entity);


Same as above

tbouron · 2017-10-04T14:17:59Z

qa/src/test/java/org/apache/brooklyn/qa/load/AbstractLoadTest.java

-                .persistMode(PersistMode.CLEAN)
-                .highAvailabilityMode(HighAvailabilityMode.MASTER)
+                .persistMode(doPersistence() ? PersistMode.CLEAN : PersistMode.DISABLED)
+                .highAvailabilityMode(doPersistence() ? HighAvailabilityMode.MASTER : HighAvailabilityMode.DISABLED)


I think for a flag, a field would be cleaner than a method to override

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.

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

ahgittin · 2017-10-04T15:22:46Z

thanks for the comments @tbouron - i've replied, and think the PR as it stands is correct. wdyt?

ahgittin · 2017-10-04T15:22:53Z

Retest this please.

tbouron · 2017-10-04T16:16:31Z

@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

ahgittin · 2017-10-04T16:29:39Z

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.

ahgittin · 2017-10-05T00:22:04Z

tests good after merging master, so merging this

ahgittin added 2 commits September 13, 2017 17:52

add load test without persistence

71a3fd2

so we can see what else is chewing up cpu

set highlights on software-specific enrichers and policies

4babb50

tbouron requested changes Oct 4, 2017

View reviewed changes

Merge branch 'master' into highlights-and-load

582d2b6

asfgit merged commit 582d2b6 into apache:master Oct 5, 2017

asfgit pushed a commit that referenced this pull request Oct 5, 2017

This closes #126

7c23ff0

Conversation

ahgittin commented Sep 13, 2017

Uh oh!

tbouron left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ahgittin commented Oct 4, 2017

Uh oh!

ahgittin commented Oct 4, 2017

Uh oh!

tbouron commented Oct 4, 2017

Uh oh!

ahgittin commented Oct 4, 2017

Uh oh!

ahgittin commented Oct 5, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants