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
chore(exporters/elasticsearch-exporter): remove high level rest client #4551
Conversation
Hey @korthout, I know we didn't talk about this before, but as menski is sick/has no time for this, I'd need someone else to review this :) Let's just quickly chat about it tomorrow before anything! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great work on getting these dependencies out of our project. Just a couple of remarks here and there.
exporters/elasticsearch-exporter/src/main/java/io/zeebe/exporter/ElasticsearchClient.java
Show resolved
Hide resolved
exporters/elasticsearch-exporter/src/main/java/io/zeebe/exporter/ElasticsearchClient.java
Outdated
Show resolved
Hide resolved
exporters/elasticsearch-exporter/src/main/java/io/zeebe/exporter/ElasticsearchClient.java
Show resolved
Hide resolved
exporters/elasticsearch-exporter/src/main/java/io/zeebe/exporter/ElasticsearchClient.java
Outdated
Show resolved
Hide resolved
...ters/elasticsearch-exporter/src/test/java/io/zeebe/exporter/util/ElasticsearchContainer.java
Outdated
Show resolved
Hide resolved
Thanks for the quick review, I applied some of the feedback; I think only the new line one is still open. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, but I did comment on some things. In particular the thrown exception is important to have a last look at before merging.
But here's the approval in advance 🎉
Updated to properly handle PutIndexTemplate requests, good catch! |
- replaces the high level rest client by the low level rest client - removes several elastic search dependencies
71040f1
to
6740ce3
Compare
bors r+ |
4551: chore(exporters/elasticsearch-exporter): remove high level rest client r=npepinpe a=npepinpe ## Description This PR removes Elasticsearch's high level REST client and replaces it with the low level REST client, primarily to remove many heavy dependencies (such as embedding the Elasticsearch server itself) from the project. Out of scope was refactoring the exporter in general, though it could benefit from it. The scope here was strictly replacing the clients, which means most requests are simply maps, with a few DTOs to more easily handle responses. ## Related issues closes #1343 # Co-authored-by: Nicolas Pépin-Perreault <nicolas.pepin-perreault@camunda.com>
Build failed |
bors retry |
Build succeeded |
) * feature(backend): add batch size throttling Related with #1590 * feature(backend): add set requestOptions for RecordsReader (for testing) Related with #1590 * feature(backend): add reset to NumberThrottleable Related with #1590 * feat(backend): clean up Related with #1590 * fix(backend): move setRequestOptions to ElasticsearchUtil Related with #1590 --------- Co-authored-by: Svetlana Dorokhova <svetlana.dorokhova@camunda.com>
Description
This PR removes Elasticsearch's high level REST client and replaces it with the low level REST client, primarily to remove many heavy dependencies (such as embedding the Elasticsearch server itself) from the project. Out of scope was refactoring the exporter in general, though it could benefit from it. The scope here was strictly replacing the clients, which means most requests are simply maps, with a few DTOs to more easily handle responses.
Related issues
closes #1343
Pull Request Checklist
mvn clean install -DskipTests
locally before committing