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

feat(elasticsearch) add new elasticsearch_v8 output #3160

Merged
merged 10 commits into from
Feb 5, 2025
Merged

Conversation

ooesili
Copy link
Contributor

@ooesili ooesili commented Jan 31, 2025

This new output uses the latest official Elasticsearch Go library.

  • Although the docs suggested it, I decided not to use esutil.NewBulkIndexer because it has a it's own internal concurrency and flushing logic that we don't really need since the benthos engine provides that for us.
  • I didn't support create action because it is not idempotent which I don't think would play well with the assumptions that Connect makes of it's plugins.

@ooesili ooesili added enhancement outputs Any tasks or issues relating specifically to outputs go Pull requests that update Go code labels Jan 31, 2025
@ooesili ooesili self-assigned this Jan 31, 2025
@CLAassistant
Copy link

CLAassistant commented Jan 31, 2025

CLA assistant check
All committers have signed the CLA.

Copy link
Collaborator

@rockwotj rockwotj left a comment

Choose a reason for hiding this comment

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

Will review the tests a bit later

CHANGELOG.md Show resolved Hide resolved
internal/impl/elasticsearchv8/output.go Outdated Show resolved Hide resolved
internal/impl/elasticsearchv8/output.go Outdated Show resolved Hide resolved
internal/impl/elasticsearchv8/output.go Outdated Show resolved Hide resolved
internal/impl/elasticsearchv8/output.go Outdated Show resolved Hide resolved
Copy link
Collaborator

@rockwotj rockwotj left a comment

Choose a reason for hiding this comment

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

Nice work! LGTM, just a couple smaller things and then we're good to go.

internal/impl/elasticsearchv8/output.go Outdated Show resolved Hide resolved
@@ -35,6 +35,7 @@ import (
_ "github.com/redpanda-data/connect/v4/public/components/dgraph"
_ "github.com/redpanda-data/connect/v4/public/components/discord"
_ "github.com/redpanda-data/connect/v4/public/components/elasticsearch"
_ "github.com/redpanda-data/connect/v4/public/components/elasticsearchv8"
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: up to you, but I would just put this in the same elasticsearch package. You could also use a v8 subdirectory if you do want to separate the packages.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was going to keep them separate because I think we won't want to support the older elasticsearch output in cloud. I'll move it into a v8 subpackage like you suggested.

internal/plugins/info.csv Outdated Show resolved Hide resolved
internal/impl/elasticsearchv8/output.go Outdated Show resolved Hide resolved
Comment on lines 350 to 353
action, err := batch.TryInterpolatedString(i, e.conf.actionStr)
if err != nil {
return fmt.Errorf("interpolating action: %w", err)
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

You should be using batch.InterpolationExecutor and reusing the executors across the batch to prevent N copies of the batch (TryInterpolatedString makes a shallow copy of the batch every time it's called).

Copy link
Collaborator

@mihaitodor mihaitodor left a comment

Choose a reason for hiding this comment

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

That looks great, nice one! Feel free to 🐑 🚀

@mihaitodor
Copy link
Collaborator

PS: Before merging, would you mind adding a note to the Changelog about retry_on_conflict being added to the elasticsearch output? I merged #3147 so we have that as well.

@ooesili ooesili merged commit e6781a0 into main Feb 5, 2025
4 checks passed
@mihaitodor mihaitodor deleted the elasticsearch-v8 branch February 5, 2025 16:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement go Pull requests that update Go code outputs Any tasks or issues relating specifically to outputs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants