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

Make alert conditions resilient against changing number formats. #2182

Merged
merged 6 commits into from May 9, 2016
Merged

Conversation

@dennisoelkers
Copy link
Member

@dennisoelkers dennisoelkers commented May 4, 2016

In 1.3.x numerical alert condition parameters were stored by MongoDB
(resp. the mongo java driver) in a way, that they are returned as
Doubles to the server, while alert conditions persisted in 2.0.0 return
numerical parameters as Integer. Therefore, 2.0.0 cannot handle alert
conditions stored in 1.3.x, so legacy installations upgraded to 2.0.0
do not check any alert conditions anymore.

This change allows to handle both formats by generally assuming they are
Number/Double types and returning their intValue().

Fixes #2169

@dennisoelkers dennisoelkers added this to the 2.0.1 milestone May 4, 2016
@dennisoelkers
Copy link
Member Author

@dennisoelkers dennisoelkers commented May 4, 2016

Adding some tests as well.

In 1.3.x numerical alert condition parameters were stored by MongoDB
(resp. the mongo java driver) in a way, that they are returned as
Doubles to the server, while alert conditions persisted in 2.0.0 return
numerical parameters as Integer. Therefore, 2.0.0 cannot handle alert
conditions stored in 1.3.x, so legacy installations upgraded to 2.0.0
do not check any alert conditions anymore.

This change allows to handle both formats by generally assuming they are
Number/Double types and returning their `intValue()`.

Fixes #2169
This also helps us, because the "threshold" parameter could also be a
double in some cases.
@@ -195,4 +187,16 @@ public NegativeCheckResult(AlertCondition alertCondition) {
}
}

@VisibleForTesting

This comment has been minimized.

@joschi

joschi May 4, 2016
Contributor

Minor semantic nitpick: Child classes are also using this (protected) method, so it's not just visible for testing. 😉

This comment has been minimized.

@dennisoelkers

dennisoelkers May 4, 2016
Author Member

@joschi joschi self-assigned this May 4, 2016

import static org.junit.Assert.assertFalse;
import static org.junit.Assert.assertTrue;
import static org.junit.Assert.*;

This comment has been minimized.

@joschi

joschi May 4, 2016
Contributor

Please don't use wildcard imports.

Configuration for IntelliJ IDEA: http://stackoverflow.com/a/3348855

This comment has been minimized.

@dennisoelkers

dennisoelkers May 4, 2016
Author Member

@joschi
Copy link
Contributor

@joschi joschi commented May 4, 2016

LGTM (except for those 2 minor nitpicks). 👍

@joschi
Copy link
Contributor

@joschi joschi commented May 9, 2016

LGTM. 👍

@joschi joschi merged commit d164e7f into 2.0 May 9, 2016
4 checks passed
4 checks passed
@garybot2
ci-server-integration Jenkins build graylog2-server-integration-pr 887 has succeeded
Details
@garybot2
ci-web-linter Jenkins build graylog-pr-linter-check 375 has succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
@joschi joschi deleted the issue-2169 branch May 9, 2016
dennisoelkers added a commit that referenced this pull request May 9, 2016
In 1.3.x numerical alert condition parameters were stored by MongoDB
(resp. the Mongo Java Driver) in a way, that they are returned as
Double  while alert conditions persisted in 2.0.0 return numerical parameters
as Integer. Therefore, 2.0.0 cannot handle alert conditions stored in 1.3.x
and legacy installations upgraded to 2.0.0 do not check any alert conditions
anymore.

This change allows to handle both formats by generally assuming they are
Number/Double types and returning their `intValue()`.

Fixes #2169
(cherry picked from commit d164e7f)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants