You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@brooklyn.apache.org by bostko <gi...@git.apache.org> on 2015/07/01 16:50:56 UTC
[GitHub] incubator-brooklyn pull request: Count the null values for the ave...
GitHub user bostko opened a pull request:
https://github.com/apache/incubator-brooklyn/pull/731
Count the null values for the average enrichers
This fixes TomcatAutoScalerPolicyTest
Basically the problem is that after making a request to Tomcat uptade event for REQUEST_COUNT_PER_NODE is triggered immediately.
This causes Enrichers.average() to have a null value for the requests of the newly created Tomcat Entity.
This leads to average of 2 requests per node which exceeds the allowed metric of 0 to 1 which triggers scaling.
It is discuss-able whether we should apply the fix here, but I think in the general case it is good to include null for average values.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/bostko/incubator-brooklyn master
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/incubator-brooklyn/pull/731.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 #731
----
commit 45f1ec1c134b17ab3860db65998ead88426769b6
Author: Valentin Aitken <va...@cloudsoftcorp.com>
Date: 2015-07-01T14:31:02Z
Count the null values for the average enrichers
This fixes TomcatAutoScalerPolicyTest
----
---
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: Count the null values for the ave...
Posted by neykov <gi...@git.apache.org>.
Github user neykov commented on the pull request:
https://github.com/apache/incubator-brooklyn/pull/731#issuecomment-122264455
LGTM - can be merged regardless of what we decide on the default value for the averaging enricher.
---
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: Count the null values for the ave...
Posted by sjcorbett <gi...@git.apache.org>.
Github user sjcorbett commented on the pull request:
https://github.com/apache/incubator-brooklyn/pull/731#issuecomment-118895383
One for the mailing list?
---
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: Count the null values for the ave...
Posted by aledsage <gi...@git.apache.org>.
Github user aledsage commented on the pull request:
https://github.com/apache/incubator-brooklyn/pull/731#issuecomment-124335224
Thanks all - 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: Count the null values for the ave...
Posted by neykov <gi...@git.apache.org>.
Github user neykov commented on the pull request:
https://github.com/apache/incubator-brooklyn/pull/731#issuecomment-119913113
Having looked at this in more detail, I still think that the averaging enricher should count the null values by default. Perhaps it could also look at the entity state and exclude stopped entities.
I agree with @sjcorbett that the place to discuss this is the dev mailing list, @bostko could you start a thread there.
---
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: Count the null values for the ave...
Posted by bostko <gi...@git.apache.org>.
Github user bostko commented on the pull request:
https://github.com/apache/incubator-brooklyn/pull/731#issuecomment-118779959
Svet suggested that everywhere we should use 0 as a default null value for the average enricher because we could often have situations where some of the entities isn't up.
One way I can think of for doing this is if we change the argument `valueToReportIfNoSensors` to `0` in Enrichers.java:268 but if we do so we have to call `defaultValueForUnreportedSensors(null)` on the enricher to get to the old behavior.
Basically we should consider how the null values will be treated especially when binding a Scaling policy to aggregated sensor.
---
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: Count the null values for the ave...
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/731#discussion_r34243108
--- Diff: software/nosql/src/test/java/brooklyn/entity/nosql/cassandra/CassandraDatacenterTest.java ---
@@ -74,6 +74,7 @@ public void testPopulatesInitialSeeds() throws Exception {
.configure(CassandraDatacenter.MEMBER_SPEC, EntitySpec.create(EmptySoftwareProcess.class)));
app.start(ImmutableList.of(loc));
+ Thread.sleep(Duration.seconds(1).toMilliseconds());
--- End diff --
Why the sleep?
---
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: Count the null values for the ave...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/incubator-brooklyn/pull/731
---
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.
---