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

Elasticsearch Java Client #22622

Merged
merged 1 commit into from
Feb 4, 2023

Conversation

loicmathieu
Copy link
Contributor

Prototype of an Elasticsearch Java Client.

As discussed on Zulip, it cannot work untill we move to jakarta EE or we find a workaround for the Caused by: java.lang.NoClassDefFoundError: jakarta/json/spi/JsonProvider

@quarkus-bot quarkus-bot bot added area/core area/elasticsearch area/infra-automation anything related to CI, bots, etc. that are used to automated our infrastructure labels Jan 4, 2022
@loicmathieu
Copy link
Contributor Author

@stuartwdouglas rumors said you may have an idea to workaround the issue of Javax vs Jakarta EE, if so, when you'll back from PTO, can you share some throughts ?

@m-stramel
Copy link

@loicmathieu what zulip conversation are you referring to? Any updates on this since the last comment?

@loicmathieu
Copy link
Contributor Author

@m-stramel I open a new discussion on Zulip https://quarkusio.zulipchat.com/#narrow/stream/187038-dev/topic/Jakarta.20JSON and well, don't expect it to be soon as the Jakarta migration will take some time.

@loicmathieu loicmathieu force-pushed the elasticsearch-java-client branch 2 times, most recently from 5d4ed03 to 7ecb472 Compare July 13, 2022 10:04
@quarkus-bot quarkus-bot bot added area/dependencies Pull requests that update a dependency file area/devtools Issues/PR related to maven, gradle, platform and cli tooling/plugins area/documentation labels Jul 13, 2022
@loicmathieu loicmathieu marked this pull request as ready for review July 13, 2022 10:54
@quarkus-bot

This comment has been minimized.

@loicmathieu loicmathieu marked this pull request as draft July 13, 2022 12:05
@loicmathieu loicmathieu marked this pull request as ready for review July 13, 2022 12:41
Copy link
Member

@yrodiere yrodiere left a comment

Choose a reason for hiding this comment

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

I think there are a few things you should remove, see below.

Otherwise, this makes sense. I just checked and the client really is opensource: https://github.com/elastic/elasticsearch-java

This client probably won't work with OpenSearch, but then we can always add another extension for OpenSearch.

However, the problem remains that these clients are forward-compatible, not backward-compatible:

The Elasticsearch Java client is forward compatible; meaning that the client supports communicating with greater or equal minor versions of Elasticsearch. Elasticsearch language clients are only backwards compatible with default distributions and without guarantees made.

Source

This means that upgrading the version of the client in Quarkus will force users to upgrade their cluster...

I don't know how acceptable this is, but we will need to discuss this before this extensions leaves preview.

@loicmathieu
Copy link
Contributor Author

@yrodiere

This client probably won't work with OpenSearch, but then we can always add another extension for OpenSearch.

This is on my todo list ;)

Elasticsearch language clients are only backwards compatible with default distributions and without guarantees made

I think the same applied to the High Level Client but was not as clearly stated as now. The Java client uses the low level rest client under the cover, as the High Level Client does. So for me it's the same caveat at the existing High Level Client.

@yrodiere
Copy link
Member

I think the same applied to the High Level Client but was not as clearly stated as now. The Java client uses the low level rest client under the cover, as the High Level Client does. So for me it's the same caveat at the existing High Level Client.

Oh certainly, that's not new. But that was one of the reasons why the High Level client never left preview. We can't reasonably guarantee backwards compatibility or timely updates in case of CVEs if that's all the client can offer us...

I don't really have a solution, just saying this is a problem and we'll need to agree on a solution.

@loicmathieu
Copy link
Contributor Author

I don't really have a solution, just saying this is a problem and we'll need to agree on a solution.

This is a limitation of this client and we don't have a solution. We can warn the user but again, as we don't have a solution and it's a decision we didn't take it's difficult to answer questions about it that will arise if we add some sentence warning about it in our guide.

To be honest, I used the high level rest client in multiple projects and we didn't had the issue as long as major versions are the same. I expect the same for the Java client (it is automatically build from the REST API so as long as the REST API didn't have breaking change the client will not have any).

Competitors framework already include this client and people will start asking for it, especially now that the high level client is stuck at 7.10. I know it's not a reason to offer a new extension but it's something to take into consideration. And it will allow us to remove the high level client extension after a few release cycle :).

So even if it's not a great solution, I think it's the better we can offer.

@loicmathieu
Copy link
Contributor Author

Native build is failing, I'll have a look in the next few days.

@quarkus-bot

This comment has been minimized.

@loicmathieu loicmathieu force-pushed the elasticsearch-java-client branch 2 times, most recently from 9943fce to 5912d08 Compare July 15, 2022 16:59
@loicmathieu
Copy link
Contributor Author

@zakkak the native build of this PR failed with the following error :

Error: com.oracle.graal.pointsto.constraints.UnresolvedElementException: Discovered unresolved method during parsing: org.elasticsearch.client.RequestOptions$Builder.setHttpAsyncResponseConsumerFactory(org.elasticsearch.client.HttpAsyncResponseConsumerFactory). This error is reported at image build time because class co.elastic.clients.transport.rest_client.RestClientOptions$Builder is registered for linking at image build time by command line

However, the method org.elasticsearch.client.RequestOptions$Builder.setHttpAsyncResponseConsumerFactory(org.elasticsearch.client.HttpAsyncResponseConsumerFactory). is present on the classpath, do you have an idea why such thing can happen and how to debug this ?

@quarkus-bot

This comment has been minimized.

@quarkus-bot

This comment has been minimized.

Copy link
Member

@yrodiere yrodiere left a comment

Choose a reason for hiding this comment

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

LGTM, though I added a suggestion regarding the preview status and I think a (very basic) documentation page would be necessary for the extension to get some visibility :)

And of course we'll have to wait for Guillaume to lift the Jakarta embargo :)

@@ -0,0 +1,14 @@
---
artifact: ${project.groupId}:${project.artifactId}:${project.version}
name: "Elasticsearch Java Client"
Copy link
Member

Choose a reason for hiding this comment

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

I didn't notice this before, but I think documentation is missing, do you think you could add a page?

It doesn't need to be extensive, even just stating what the extension is, what the entry point is (the client class), and including the configuration reference would be a start.

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 planned to add a documentation and a quickstart later on but as this PR will be on ice for some time I'll do it in the same PR.

The documentation will be on the already existing Elasticsearch guide and I'll update the section about the high level client to explain that the java one is prefered.

@quarkus-bot

This comment has been minimized.

@quarkus-bot

This comment has been minimized.

@gsmet
Copy link
Member

gsmet commented Feb 4, 2023

I rebased on top of the Jakarta transformation.

@gsmet gsmet added triage/needs-rebase This PR needs to be rebased first because it has merge conflicts and removed triage/needs-rebase This PR needs to be rebased first because it has merge conflicts labels Feb 4, 2023
@quarkus-bot
Copy link

quarkus-bot bot commented Feb 4, 2023

Failing Jobs - Building d7f217b

Status Name Step Failures Logs Raw logs
✔️ JVM Tests - JDK 11
JVM Tests - JDK 11 Windows Build Failures Logs Raw logs
✔️ JVM Tests - JDK 17
✔️ JVM Tests - JDK 18
Native Tests - Messaging1 Build ⚠️ Check → Logs Raw logs

Full information is available in the Build summary check run.

Failures

⚙️ JVM Tests - JDK 11 Windows #

- Failing: extensions/resteasy-reactive/rest-client-reactive/deployment 
! Skipped: extensions/keycloak-admin-client-reactive/deployment extensions/oidc-client-reactive-filter/deployment extensions/oidc-token-propagation-reactive/deployment and 19 more

📦 extensions/resteasy-reactive/rest-client-reactive/deployment

io.quarkus.rest.client.reactive.stork.StorkResponseTimeLoadBalancerTest.shouldUseFasterService line 60 - More details - Source on GitHub

org.opentest4j.AssertionFailedError: 

expected: "hello, Alice"

@gsmet
Copy link
Member

gsmet commented Feb 4, 2023

@loicmathieu I'm going to merge this one. Could you prepare some basic documentation before we release 3.0? Thanks!

@gsmet gsmet merged commit eabef0f into quarkusio:main Feb 4, 2023
@quarkus-bot quarkus-bot bot added this to the 3.0 - main milestone Feb 4, 2023
@loicmathieu
Copy link
Contributor Author

@gsmet thanks for the rebase. Sure I will prepare some documentation and quickstarts for this one.

@yrodiere
Copy link
Member

yrodiere commented Mar 6, 2023

Hey @loicmathieu, did you find the time to work on documentation? I couldn't find it.
Thanks.

@loicmathieu
Copy link
Contributor Author

@yrodiere no, I plan to work on it this week. Don't hesitate to ping me if you didn't see the PR for it this week ;)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/core area/dependencies Pull requests that update a dependency file area/devtools Issues/PR related to maven, gradle, platform and cli tooling/plugins area/documentation area/elasticsearch area/infra-automation anything related to CI, bots, etc. that are used to automated our infrastructure release/noteworthy-feature triage/flaky-test
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants