Skip to content

Conversation

mashhurs
Copy link
Contributor

@mashhurs mashhurs commented Aug 8, 2025

What this PR does?

It introduces GeoIP bridge interfaces where Logstash elastic_integration plugin can utilize.

  • Have you signed the contributor license agreement?
  • Have you followed the contributor guidelines?
  • If submitting code, have you built your formula locally prior to submission with gradle check?
  • If submitting code, is your pull request against main? Unless there is a good reason otherwise, we prefer pull requests against main and will backport as needed.
  • If submitting code, have you checked that your submission is for an OS and architecture that we support?
  • If you are submitting this code for a class then read our policy for that.

@elasticsearchmachine elasticsearchmachine added v9.2.0 external-contributor Pull request authored by a developer outside the Elasticsearch team labels Aug 8, 2025
@mashhurs mashhurs force-pushed the logstash-bridge-geoip-interfaces branch from bf88d27 to 65d9b9c Compare August 8, 2025 18:14
@mashhurs mashhurs marked this pull request as ready for review August 8, 2025 19:25
@mashhurs mashhurs requested a review from a team as a code owner August 8, 2025 19:25
@elasticsearchmachine elasticsearchmachine added the needs:triage Requires assignment of a team area label label Aug 8, 2025
@mashhurs mashhurs added Team:Core/Infra Meta label for core/infra team and removed external-contributor Pull request authored by a developer outside the Elasticsearch team labels Aug 8, 2025
@elasticsearchmachine elasticsearchmachine removed the Team:Core/Infra Meta label for core/infra team label Aug 8, 2025
@elasticsearchmachine elasticsearchmachine removed the needs:triage Requires assignment of a team area label label Aug 8, 2025
@mashhurs mashhurs force-pushed the logstash-bridge-geoip-interfaces branch from fea4f18 to 7eb593c Compare August 11, 2025 19:37
Copy link
Member

@yaauie yaauie left a comment

Choose a reason for hiding this comment

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

While there's a lot going on here, I'm pretty confident in the general shape and the downstream project is passing CI when pointed at these changes.

  • all bridges are interfaces, and provide factory methods to create instances from bridged inputs, making the ProxyInternal-ish classes locked-down implementation details of this bridge that are not exposed to consumers of the bridge.
  • we provide AbstractExternal...Bridge base classes for implementing bridged interfaces externally without needing to know about Elasticsearch-internal types.

exports org.elasticsearch.ingest.geoip.stats to org.elasticsearch.server;

exports org.elasticsearch.ingest.geoip to com.maxmind.db;
exports org.elasticsearch.ingest.geoip to com.maxmind.db, org.elasticsearch.logstashbridge;
Copy link
Member

Choose a reason for hiding this comment

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

I can understand that this needs to export to org.elasticsearch.logstashbridge, but am unsure why it needs to export to com.maxmind.db, which I believe to be a dependency of this module (not dependent on this module).

I'm okay paring this off as a separate thing to chase down.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oops! Your assumption is correct and I believe we should remove this. I will do it now as I don't see any harmful side affects to our changes.

@yaauie
Copy link
Member

yaauie commented Sep 5, 2025

1 failing check

Check labels — Missing: [:Team]

While we're tagged Team:Logstash, the failing check is looking for a tag that begins with a : (the ES short-hand for team ownership)

@mashhurs mashhurs merged commit cfb3434 into elastic:main Sep 5, 2025
33 of 34 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

3 participants