Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

BROOKLYN-569: aggregator casts vals to numbers #915

Merged
merged 2 commits into from
Dec 21, 2017

Conversation

aledsage
Copy link
Contributor

No description provided.

@aledsage
Copy link
Contributor Author

Retest this please

Test failure is unrelated: ElectPrimaryTest.testSelectionModeFailoverReelectWithPreference. The info log from jenkins has the lines shown below. This tells us that ElectPrimaryEffector is being called twice - once directly int the test, and presumably concurrently by the policy's onEvent (it triggers that whenever a child/member is added/removed or its serviceUp/serviceState/weighting sensor changes). There is a definite race there, which can happen when the effector is being called manually.

2017-12-11 17:28:13,488 INFO  Test created app, and will now start BasicApplicationImpl{id=jt5d932r4f}
2017-12-11 17:28:13,515 INFO  Detected new primary TestEntityImpl{id=jt0dh78cvw} at BasicApplicationImpl{id=jt5d932r4f} (previously had null)
2017-12-11 17:28:13,515 INFO  Primary TestEntityImpl{id=jt0dh78cvw} at BasicApplicationImpl{id=jt5d932r4f} detected as healthy
2017-12-11 17:28:13,528 INFO  Started application BasicApplicationImpl{id=jt5d932r4f}
2017-12-11 17:28:13,530 INFO  Detected new primary TestEntityImpl{id=ndkegu8wyc} at BasicApplicationImpl{id=jt5d932r4f} (previously had TestEntityImpl{id=jt0dh78cvw})
2017-12-11 17:28:13,531 INFO  Detected new primary TestEntityImpl{id=jt0dh78cvw} at BasicApplicationImpl{id=jt5d932r4f} (previously had TestEntityImpl{id=ndkegu8wyc})
2017-12-11 17:28:43,544 INFO  failed succeeds-eventually, 75 attempts, 30000ms elapsed (rethrowing): java.lang.AssertionError: attribute=Sensor: primary (org.apache.brooklyn.api.entity.Entity); val=TestEntityImpl{id=jt0dh78cvw}

for (Object val : vals) {
Maybe<Number> coercedVal = TypeCoercions.tryCoerce(val, Number.class);
if (coercedVal.isPresentAndNonNull()) {
postProcessedVals.add(coercedVal.get());
count++;
} else if (defaultValueForUnreportedSensors != null) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

worth a log.warn saying the value if not coercible?

} else {
  if (val!=null) {
    log.warn("Input to numeric aggregator is not a number: "+val+" ("+val.getClass+")");
  }
  if (defaultValueForUnrepoertedSensors != null) {
    ...
  }
}

Copy link
Contributor Author

@aledsage aledsage Dec 21, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ahgittin Done - but also guarding so we don't log repeatedly at warn. The aggregator might well get called every few seconds per enricher, so must avoid flooding the log with such warn messages.

@ahgittin
Copy link
Contributor

@aledsage minor comment, merge when ready

@tbouron
Copy link
Member

tbouron commented Dec 21, 2017

Tested, LGTM, thanks @aledsage

@asfgit asfgit merged commit e723851 into apache:master Dec 21, 2017
asfgit pushed a commit that referenced this pull request Dec 21, 2017
@aledsage aledsage deleted the BROOKLYN-569 branch December 21, 2017 14:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants