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

Upgrade geoip2 dependency #93522

Merged
merged 9 commits into from
Feb 7, 2023

Conversation

joegallo
Copy link
Contributor

@joegallo joegallo commented Feb 6, 2023

@joegallo joegallo added >non-issue :Data Management/Ingest Node Execution or management of Ingest Pipelines including GeoIP Team:Data Management Meta label for data/management team v8.7.0 labels Feb 6, 2023
@elasticsearchmachine
Copy link
Collaborator

Pinging @elastic/es-data-management (Team:Data Management)

Copy link
Contributor

@pgomulka pgomulka left a comment

Choose a reason for hiding this comment

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

LGTM

@ChrisHegarty
Copy link
Contributor

LGTM. (good to merge, from my POV)

Just a note on the addition of requires java.net.http:

  1. maxmind/GeoIP2-java has triggered the need for the java.net.http module. See the release note [1]. The release note says that the use of Apache HttpClient has been replaced with java.net.http.HttpClient, but strangely I see no previous Gradle (or otherwise) dependency on the Apache HttpClient. Maybe we never exercise this particular functionality of maxmind/GeoIP2-java ?

  2. Server does not strictly require java.net.http, since server does not currently use anything from the java.net.http module. The use of requires here simply ensures that the java.net.http is in the boot layer, which allows the geoip plugin to access it there. Alternatively, we could add java.net.http to the --add-modules in the CLI, see ServerProcess.java [2]

  3. Neither the requires on server, nor the --add-modules, should be necessary. The plugin layer should find JDK modules, not in the boot layer, in the JDK module finder. There could be a bug here, but I thought that this "just worked" - I'll take a look.

[1] https://github.com/maxmind/GeoIP2-java/blob/43c354f22410d1ef58f26c2653c619853c373bef/CHANGELOG.md?plain=1#L26
[2] https://github.com/elastic/elasticsearch/blob/3f47fe9422aa5166b6b4a793ede5e51c4b794805/distribution/tools/server-cli/src/main/java/org/elasticsearch/server/cli/ServerProcess.java#LL220C24-L220C30

@joegallo
Copy link
Contributor Author

joegallo commented Feb 7, 2023

Maybe we never exercise this particular functionality of maxmind/GeoIP2-java?

Yeah, that's my understanding. We download the files ourselves and then manage them locally via indexes and files, from the POV of the geoip library, there's no downloading to do.

@joegallo joegallo merged commit ea17a19 into elastic:main Feb 7, 2023
@joegallo joegallo deleted the ingest-geoip-maxmind-version-400 branch February 7, 2023 13:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
:Data Management/Ingest Node Execution or management of Ingest Pipelines including GeoIP >non-issue Team:Data Management Meta label for data/management team v8.7.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants