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.

Sign up for GitHub

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

Jump to bottom

Enable ASN support for Ingest GeoIP plugin. #27958

Merged
merged 1 commit into from
Jan 5, 2018
Merged

Enable ASN support for Ingest GeoIP plugin. #27958

merged 1 commit into from
Jan 5, 2018

Conversation

kiawin
Copy link
Contributor

@kiawin kiawin commented Dec 22, 2017

Closes #27849

@martijnvg martijnvg self-assigned this Jan 4, 2018
@martijnvg
Copy link
Member

@elasticmachine please test this

@elasticmachine
Copy link
Collaborator

Since this is a community submitted pull request, a Jenkins build has not been kicked off automatically. Can an Elastic organization member please verify the contents of this patch and then kick off a build manually?

1 similar comment
@elasticmachine
Copy link
Collaborator

Since this is a community submitted pull request, a Jenkins build has not been kicked off automatically. Can an Elastic organization member please verify the contents of this patch and then kick off a build manually?

martijnvg
martijnvg requested changes Jan 4, 2018
View reviewed changes
Copy link
Member

@martijnvg martijnvg left a comment

Choose a reason for hiding this comment

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

@kiawin I left some comments. Looks good in general.

plugins/ingest-geoip/src/test/resources/rest-api-spec/test/ingest_geoip/20_geoip_processor.yml Outdated
- match: { _source.field1: "82.170.213.79" }
- length: { _source.geoip: 3 }
- match: { _source.geoip.ip: "82.170.213.79" }
- match: { _source.geoip.asn: "5615" }
Copy link
Member

Choose a reason for hiding this comment

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

This should be 5615 instead of "5615".

plugins/ingest-geoip/src/test/resources/rest-api-spec/test/ingest_geoip/20_geoip_processor.yml Outdated
- length: { _source.geoip: 3 }
- match: { _source.geoip.ip: "82.170.213.79" }
- match: { _source.geoip.asn: "5615" }
- match: { _source.geoip.organization_name: "KPN B.V." }
Copy link
Member

Choose a reason for hiding this comment

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

a new line at the end of this file is required.

buildSrc/version.properties Outdated
@@ -4,7 +4,7 @@ lucene = 7.2.0
# optional dependencies
spatial4j = 0.6
jts = 1.13
jackson = 2.8.10
jackson = 2.9.3
Copy link
Member

Choose a reason for hiding this comment

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

Unfortunately ES can't be upgraded to the latest Jackson 2.9.x release: #27032 (comment)

But I think ASN support will still work with Jackson 2.8.10, so lets undo the Jackson upgrade here.

plugins/ingest-geoip/build.gradle Outdated
@@ -23,8 +23,7 @@ esplugin {
}

dependencies {
// Upgrade to 2.10.0 or higher when jackson-core gets upgraded to 2.9.x
compile ('com.maxmind.geoip2:geoip2:2.9.0')
compile ('com.maxmind.geoip2:geoip2:2.10.0')
Copy link
Member

Choose a reason for hiding this comment

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

This library upgrade then also needs to be revered.

@kiawin
Copy link
Contributor Author

kiawin commented Jan 4, 2018

@martijnvg done the changes. back to you :)

martijnvg
martijnvg approved these changes Jan 5, 2018
View reviewed changes
Copy link
Member

@martijnvg martijnvg left a comment

Choose a reason for hiding this comment

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

Thanks @kiawin! This PR looks good to me now.

@martijnvg martijnvg force-pushed the 27849-ingest-goeip-support-asn branch from f3403e7 to 21ebbcf Compare January 5, 2018 10:08
@martijnvg martijnvg force-pushed the 27849-ingest-goeip-support-asn branch from 21ebbcf to 81179c4 Compare January 5, 2018 10:09
@martijnvg
Copy link
Member

@elasticmachine please test this

@martijnvg martijnvg force-pushed the 27849-ingest-goeip-support-asn branch from 81179c4 to a4a7150 Compare January 5, 2018 13:07
@martijnvg martijnvg merged commit a4a7150 into elastic:master Jan 5, 2018
@clintongormley clintongormley added :Data Management/Ingest Node Execution or management of Ingest Pipelines including GeoIP and removed :Plugin Ingest GeoIp labels Feb 13, 2018
@danielmitterdorfer danielmitterdorfer mentioned this pull request Mar 12, 2018
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@martijnvg martijnvg martijnvg approved these changes

Assignees

@martijnvg martijnvg

Labels
:Data Management/Ingest Node Execution or management of Ingest Pipelines including GeoIP >feature v6.2.0 v7.0.0-beta1
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@kiawin @martijnvg @elasticmachine @clintongormley @colings86