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

[logstash bridge]: bootstrap #107973

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from
Draft

Conversation

yaauie
Copy link
Member

@yaauie yaauie commented Apr 26, 2024

Adds a new logstash-bridge project in /libs that:

  • publishes bridge methods for accessing various constructors, static functions, and static fields that are required by Logstash's Elastic Integration Filter (these are not a part of the distributed artifact; the LS plugin will build and use directly)
  • adds "duck type" interface validation tests to ensure that the object methods that Logstash's Elastic Integration Filter rely on maintain a stable API.

TODO:

  • hook :libs:elasticsearch-logstash-bridge:test into the build/CI, so that when someone breaks an API Logstash relies on we have direct feedback.
  • map codeowners to a public team

Adds a new `logstash-bridge` project in `/libs` that:
 - publishes bridge methods for accessing various constructors, static
   functions, and static fields that are required by Logstash's Elastic
   Integration Filter
 - adds "duck type" interface validation tests to ensure that the
   object methods that Logstash's Elastic Integration Filter rely on
   maintain a stable API.
@yaauie yaauie requested a review from rjernst April 26, 2024 21:24
@yaauie yaauie requested a review from a team as a code owner April 26, 2024 21:24
@elasticsearchmachine elasticsearchmachine added external-contributor Pull request authored by a developer outside the Elasticsearch team v8.15.0 needs:triage Requires assignment of a team area label labels Apr 26, 2024
@yaauie yaauie added the :Core/Infra/Core Core issues without another label label Apr 26, 2024
@elasticsearchmachine elasticsearchmachine added Team:Core/Infra Meta label for core/infra team and removed needs:triage Requires assignment of a team area label labels Apr 26, 2024
@elasticsearchmachine
Copy link
Collaborator

Pinging @elastic/es-core-infra (Team:Core/Infra)

@yaauie yaauie added >enhancement needs:triage Requires assignment of a team area label and removed Team:Core/Infra Meta label for core/infra team labels Apr 26, 2024
@elasticsearchmachine elasticsearchmachine added Team:Core/Infra Meta label for core/infra team and removed needs:triage Requires assignment of a team area label labels Apr 26, 2024
@elasticsearchmachine
Copy link
Collaborator

Hi @yaauie, I've created a changelog YAML for you.

@mark-vieira
Copy link
Contributor

  • publishes bridge methods for accessing various constructors, static functions, and static fields that are required by Logstash's Elastic Integration Filter (these are not a part of the distributed artifact; the LS plugin will build and use directly)

Is this a new or existing plugin? Is that plugin going to be part of this repository or do we need to publish this artifact somewhere for external consumption?

@yaauie
Copy link
Member Author

yaauie commented Apr 29, 2024

Is this a new or existing plugin? Is that plugin going to be part of this repository or do we need to publish this artifact somewhere for external consumption?

The elastic/logstash-filter-elastic_integration exists, and is a plugin for Logstash, whose artifacts are published as self-contained rubygems (no maven). The plugin already builds and bundles a minimal subset of Elasticsearch as part of its build process, and is capable of also building this artifact for self-consumption without requiring it to be published elsewhere.

apply plugin: 'elasticsearch.build'

dependencies {
compileOnly project(':server')
Copy link
Contributor

Choose a reason for hiding this comment

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

@rjernst I vaguely remember us not wanting libs to depend on server but this isn't really a "lib" of traditional sorts.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm glad to place it elsewhere, even in x-pack project as this does not need to be published under an OSS license. Discussions with @rjernst offline indicated that /libs was an appropriate starting-point.

[The] way i would do this is have a lib, it will depend on server and painless. That lib will only live inside ES source, you can build/pull in the jars within logstash
-- @rjernst via Slack

Copy link
Member

Choose a reason for hiding this comment

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

This is a tricky and special case. I think we are ok to depend on server here.

compileOnly project(':modules:lang-painless:spi')

testImplementation(project(":test:framework")) {
exclude group: 'org.elasticsearch', module: 'logstashbridge'
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is this required? Nothing should actually depend on this project so there should be no reason to exclude it.

Copy link
Member Author

Choose a reason for hiding this comment

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

Looks lime a remnant from copying a different lib to bootstrap. I'll snip.


public class ThreadPoolBridgeTests extends ESTestCase {

public void testThreadPoolAPIStability() throws Exception {
Copy link
Member

Choose a reason for hiding this comment

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

I don't understand the purpose of these tests. The point of this lib is that it is a bridge that will maintain all compatibility promises. If you need access to ThreadPool, have your own ThreadPool wrapper, and delegate in this bridge lib to the ES ThreadPool. By doing that, any refactorings (eg renamings/adjustments/whatever) of ThreadPool methods will require ES devs to update this code before merging, no notification to logstash devs necessary.

Copy link
Member Author

Choose a reason for hiding this comment

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

Got it.

For many of these objects, we need to pass actual instances of these objects to ES-owned code, or to work with instances that are provided by ES-owned code.\

I had hoped to avoid many short-lived wrappers for each object by applying some pressure to the stability of the API, but I understand now that we are not to expect that to work.

I'll convert this PR back to 'draft' and work up a wrapper pattern for each of the objects we need to work with.

@yaauie yaauie marked this pull request as draft April 30, 2024 03:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
:Core/Infra/Core Core issues without another label >enhancement external-contributor Pull request authored by a developer outside the Elasticsearch team Team:Core/Infra Meta label for core/infra team v8.15.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants