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

Add migration to fix parameter value types for alert conditions #3340

Merged
merged 3 commits into from Jan 12, 2017

Conversation

@bernd
Copy link
Member

@bernd bernd commented Jan 11, 2017

At one point at the beginning of the 2.2 development the values for integer parameters have been stored as strings in MongoDB. This has been fixed but existing broken documents need to be fixed.

@bernd bernd added this to the 2.2.0 milestone Jan 11, 2017
@edmundoa edmundoa self-assigned this Jan 11, 2017
Copy link
Member

@edmundoa edmundoa left a comment

Other than the issues in the comments, the changes seem to work and the tests also pass.

if (!(fieldValue instanceof String)) {
LOG.warn("Field <{}> in alert condition <{}> of stream <{}> is not a string but a <{}>, not trying to convert it!",
field, alertConditionId, streamId, fieldValue.getClass().getCanonicalName());
return;

This comment has been minimized.

@edmundoa

edmundoa Jan 11, 2017
Member

I think in this case we should only log the situation and continue converting other fields. This situation may happen with alert conditions created before 2.0. See ea4aa95 for more information.

This comment has been minimized.

@bernd

bernd Jan 12, 2017
Author Member

Oh, good catch! That should have been a continue.

}

if (!(fieldValue instanceof String)) {
LOG.warn("Field <{}> in alert condition <{}> of stream <{}> is not a string but a <{}>, not trying to convert it!",

This comment has been minimized.

@edmundoa

edmundoa Jan 11, 2017
Member

Could we also add the alert condition title if available?

final String stringValue = parameters.get(field, String.class);
final Integer intValue = Ints.tryParse(stringValue);

LOG.info("Converting value for field <{}> from string to integer in alert condition <{}> of stream <{}>", field, alertConditionId, streamId);

This comment has been minimized.

@edmundoa

edmundoa Jan 11, 2017
Member

I would also add the alert condition title here.

At one point at the beginning of the 2.2 development the values for
integer parameters have been stored as strings in MongoDB. This has been
fixed but existing broken documents need to be fixed.
@bernd bernd force-pushed the alert-condition-migration branch from cea9826 to 2e87fa3 Jan 12, 2017
@bernd
Copy link
Member Author

@bernd bernd commented Jan 12, 2017

Addressed all comments. Thanks!

Copy link
Member

@edmundoa edmundoa left a comment

LGTM 👍

@edmundoa edmundoa merged commit e2b0f36 into master Jan 12, 2017
4 checks passed
4 checks passed
ci-web-linter Jenkins build graylog-pr-linter-check 1273 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
licence/cla Contributor License Agreement is signed.
Details
@edmundoa edmundoa deleted the alert-condition-migration branch Jan 12, 2017
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
You can’t perform that action at this time.