HSEARCH-5464 Pluggable rest clients in the elasticsearch backend #4798

marko-bekhta · 2025-09-09T12:16:18Z

https://hibernate.atlassian.net/browse/HSEARCH-5464

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license
and can be relicensed under the terms of the LGPL v2.1 license in the future at the maintainers' discretion.
For more information on licensing, please check here.

marko-bekhta

Hey @yrodiere @lucamolteni 🙂 👋🏻

I've added some comments here and there in this PR in places that may be of interest.
Otherwise there's a lot of "noise", from moving packages and renaming.

This PR doesn't include the docs+migration guide updates. I was thinking about sending that separately, as this already has a lot of changes 🙂.

marko-bekhta · 2025-10-16T07:38:53Z

...java/org/hibernate/search/backend/elasticsearch/client/common/spi/ElasticsearchResponse.java

 	private final JsonObject body;

-	public ElasticsearchResponse(HttpHost host, int statusCode, String statusMessage, JsonObject body) {
+	public ElasticsearchResponse(String host, int statusCode, String statusMessage, JsonObject body) {


This one is pretty straightforward, we only use the host to log the response, and it's an SPI ... so removing the apache class here should be "safe"

If you're breaking this, it might be a good idea to rename it as well? In particular I see this string includes the port, so it would be more "hostAndPort"

marko-bekhta · 2025-10-16T07:42:11Z

...ain/java/org/hibernate/search/backend/elasticsearch/client/common/gson/spi/GsonProvider.java

+	 * https://github.com/google/gson/issues/764 is supposedly fixed.
+	 * Hence, we may not need the workaround anymore.
+	 * This version will be used in the test so we'll notice if things are still broken, for the main code
+	 * the workaround is in place through the GsonProviderHelper.


This GSON issue was fixed, but I kept the workaround for the main code "just in case".

If it's fixed and we have a non-regression test, I'd say just remove the workaround?

...bernate/search/backend/elasticsearch/client/ElasticsearchHttpClientConfigurationContext.java

...tiontest/backend/elasticsearch/client/ElasticsearchClientFactoryImplElasticsearchJavaIT.java

marko-bekhta · 2025-10-16T07:59:38Z

...te/search/backend/elasticsearch/client/java/ElasticsearchHttpClientConfigurationContext.java

+@SuppressJQAssistant(
+		reason = "Apache HTTP Client 5 uses a lot of classes/interfaces in the impl packages to create builders/instances etc. "
+				+ "So while it is bad to expose impl types ... in this case it's what Apache Client expects users to do?")
 public interface ElasticsearchHttpClientConfigurationContext {


More of an FYI 🙈

org.apache.hc.client5.http.impl.async.HttpAsyncClientBuilder

Maybe ignore Apache HTTP Client 5 classes in the JQAssistant rules instead? If it's really their package naming convention which is at fault.

...search/backend/elasticsearch/client/common/cfg/ElasticsearchBackendClientCommonSettings.java

...e/search/backend/elasticsearch/client/common/spi/ElasticsearchRequestInterceptorContext.java

marko-bekhta · 2025-10-16T08:37:27Z

...ernate/search/backend/elasticsearch/client/rest5/impl/ClientRest5HttpRequestInterceptor.java

+			if ( entity instanceof AsyncEntityProducer producer ) {
+				if ( !producer.isRepeatable() ) {
+					throw new AssertionFailure( "Cannot sign AWS requests with non-repeatable entities" );
+				}
+				return new HttpAsyncEntityProducerInputStream( producer, 1024 );
+			}


This one is a bit annoying as there seems to be no easier/simpler way to get the entity out of the request, but well ... it is what it is 🙂 🤷🏻

IIUC you copy-pasted the old HttpAsyncContentProducerInputStream two times where you needed it? And possibly other related classes?

This makes me uneasy, because this is very fragile code, and my memories of working with it are... not pleasant ones.

Would it make sense to abstract over the various HTTPD classes (content encoder, ...) and move this code to a single implementation in -utils instead?

marko-bekhta · 2025-10-16T08:45:09Z

...a/org/hibernate/search/backend/elasticsearch/client/opensearch/rest/impl/GsonHttpEntity.java

+ *
+ * @author Sanne Grinovero (C) 2017 Red Hat Inc.
+ */
+final class GsonHttpEntity implements HttpEntity, AsyncEntityProducer {


the apache client 5 apis changed a bit so these gson entities were updated a bit 🙂
the tests are passing and all seems good, but if anyone would want to have an extra look, that would be nice 🙂

I honestly doubt anyone can spot mistakes in this code just by reading it. Not a reproach, just an opinion: I've had to deal with this code in the past...

I hope the tests are good :)

...ava/org/hibernate/search/backend/elasticsearch/client/ElasticsearchHttpClientConfigurer.java

yrodiere

I can't review it all but I went through your comments and a bit more, here's what I have to say :)

It's really a shame we can't have a dependency from clients to backend...

This PR doesn't include the docs+migration guide updates. I was thinking about sending that separately, as this already has a lot of changes 🙂.

Since the docs changes would be in very isolated files, you might as well add these changes here :)

yrodiere · 2025-10-23T11:58:09Z

...ain/java/org/hibernate/search/backend/elasticsearch/client/common/gson/spi/GsonProvider.java

+	 * https://github.com/google/gson/issues/764 is supposedly fixed.
+	 * Hence, we may not need the workaround anymore.
+	 * This version will be used in the test so we'll notice if things are still broken, for the main code
+	 * the workaround is in place through the GsonProviderHelper.


If it's fixed and we have a non-regression test, I'd say just remove the workaround?

yrodiere · 2025-10-23T12:39:30Z

backend/elasticsearch-client/elasticsearch-java-client/pom.xml

+    <artifactId>hibernate-search-backend-elasticsearch-client-java</artifactId>
+
+    <name>Hibernate Search Backend - Elasticsearch client based on the low-level Elasticsearch java client</name>
+    <description>Hibernate Search Elasticsearch client based on the low-level Elasticsearch java client</description>


You should probably rename this, and change the description and dependency, to use that new separate artifact instead?

+1 yes, that's the plan, was just waiting for it to get released 👍🏻 🤞🏻 🙂

yrodiere · 2025-10-23T12:41:58Z

...te/search/backend/elasticsearch/client/java/ElasticsearchHttpClientConfigurationContext.java

+@SuppressJQAssistant(
+		reason = "Apache HTTP Client 5 uses a lot of classes/interfaces in the impl packages to create builders/instances etc. "
+				+ "So while it is bad to expose impl types ... in this case it's what Apache Client expects users to do?")
 public interface ElasticsearchHttpClientConfigurationContext {


Maybe ignore Apache HTTP Client 5 classes in the JQAssistant rules instead? If it's really their package naming convention which is at fault.

yrodiere · 2025-10-23T12:44:28Z

backend/elasticsearch-client/elasticsearch-rest-client/pom.xml

+    <artifactId>hibernate-search-backend-elasticsearch-client-rest</artifactId>
+
+    <name>Hibernate Search Backend - Elasticsearch client based on the low-level Elasticsearch client</name>
+    <description>Hibernate Search Elasticsearch client based on the low-level Elasticsearch client</description>


Since you know this will disappear at some point, maybe pick a more descriptive name that wouldn't imply "that's how you do rest"? E.g. -rest4 or -rest-apache-http-4?
Good idea for other clients as well, BTW...

yrodiere · 2025-10-23T12:48:34Z

...java/org/hibernate/search/backend/elasticsearch/client/common/spi/ElasticsearchResponse.java

 	private final JsonObject body;

-	public ElasticsearchResponse(HttpHost host, int statusCode, String statusMessage, JsonObject body) {
+	public ElasticsearchResponse(String host, int statusCode, String statusMessage, JsonObject body) {


If you're breaking this, it might be a good idea to rename it as well? In particular I see this string includes the port, so it would be more "hostAndPort"

yrodiere · 2025-10-23T13:15:51Z

...c/main/java/org/hibernate/search/backend/elasticsearch/impl/ElasticsearchBackendFactory.java

+					);
+				}
 			}
+			ConfigurationLog.INSTANCE.backendClientFactory( clientFactoryHolder, eventContext.render() );


Maybe pass the eventContext instead of calling render explicitly? That way there's no rendering happening if the log level is filtered out.

See for example

hibernate-search/backend/lucene/src/main/java/org/hibernate/search/backend/lucene/logging/impl/LuceneMiscLog.java

Lines 60 to 62 in c9c39e3

@Message(id = ID_OFFSET_LEGACY_ENGINE + 52,

value = "An index writer operation failed. Resetting the index writer and forcing release of locks. %1$s")

void indexWriterResetAfterFailure(@FormatWith(EventContextFormatter.class) EventContext context);

yrodiere · 2025-10-23T13:17:28Z

...c/main/java/org/hibernate/search/backend/elasticsearch/impl/ElasticsearchBackendFactory.java

+					throw ConfigurationLog.INSTANCE.backendClientFactoryMultipleConfigured(
+							clientFactoryReferences.stream().map( ref -> ref.resolve( beanResolver ) )
+									.toList(),
+							eventContext.render()


You can pass the event context directly as a parameter to the exception's constructor, it will behave better.

See for example

hibernate-search/backend/elasticsearch/src/main/java/org/hibernate/search/backend/elasticsearch/logging/impl/ConfigurationLog.java

Lines 56 to 59 in 2efc2d6

@Message(id = ID_OFFSET + 16,

value = "Invalid tenant identifiers: '%1$s'."

+ " No tenant identifier is expected, because multi-tenancy is disabled for this backend.")

SearchException tenantIdProvidedButMultiTenancyDisabled(Set<String> tenantIds, @Param EventContext context);

...search/backend/elasticsearch/client/common/cfg/ElasticsearchBackendClientCommonSettings.java

yrodiere · 2025-10-23T13:26:37Z

backend/elasticsearch-client/opensearch-rest-client/pom.xml

+        <version>8.2.0-SNAPSHOT</version>
+        <relativePath>../../../build/parents/public</relativePath>
+    </parent>
+    <artifactId>hibernate-search-backend-elasticsearch-client-opensearch</artifactId>


Shouldn't new clients be marked as incubating? "To be safe"? :)

Meaning @Inbucating in APIs/SPIs and an admonition in the docs.

yrodiere · 2025-10-23T13:29:18Z

backend/elasticsearch-client/common/pom.xml

+        <version>8.2.0-SNAPSHOT</version>
+        <relativePath>../../../build/parents/public</relativePath>
+    </parent>
+    <artifactId>hibernate-search-backend-elasticsearch-client-common</artifactId>


This module even existing feels very wrong... So we really don't have a solution to break circular dependencies between backend and clients?

Some hack to add the dependency from backend to the default client manually, directly in the POM, perhaps?

marko-bekhta · 2025-10-23T13:42:20Z

Thanks for having a look!

Since the docs changes would be in very isolated files, you might as well add these changes here :)

ohh I already did 🫣 🙂 here:
2e6dca2

at the time I didn't want to prevent anyone from looking at the PR waiting for the docs, but as I had some "free time" since then 😁 I managed to push the change here 😄

yrodiere · 2025-10-23T13:53:03Z

I had some "free time"

I envy your time management skills. Honestly.

yrodiere

Some more comments on the docs. Looks great :)

yrodiere · 2025-10-23T14:11:17Z

documentation/src/main/asciidoc/migration/index.adoc

 The https://hibernate.org/community/compatibility-policy/#code-categorization[API]
 in Hibernate Search {hibernateSearchVersion}
 is, in general, backward-compatible with Hibernate Search {hibernateSearchPreviousStableVersionShort}.
+But, there are some deprecations:


Suggested change

But, there are some deprecations:

However, some elements are now deprecated:

yrodiere · 2025-10-23T14:11:55Z

documentation/src/main/asciidoc/migration/index.adoc

+* `org.hibernate.search.backend.elasticsearch.client.ElasticsearchHttpClientConfigurer` is deprecated for removal.
+With introduction of the pluggable Elasticsearch backend clients, configurers are a part of the particular client
+and expose client-specific configuration knobs.
+* `org.hibernate.search.backend.elasticsearch.client.ElasticsearchHttpClientConfigurationContext` is also up for removal,
+as it's a part of the configurer API.


Maybe suggest which interface to use for people sticking to the same client? I.e. the simplest migration path.

yrodiere · 2025-10-23T14:15:44Z

documentation/src/main/asciidoc/public/reference/_backend-elasticsearch.adoc


 An Elasticsearch backend communicates with an Elasticsearch cluster through a REST client.
-Below are the options that affect this client.
+Hibernate Search comes with a range of pluggable Elasticsearch REST clients that the user can pick from:


"User" means something different for us and for... our users.

Suggested change

Hibernate Search comes with a range of pluggable Elasticsearch REST clients that the user can pick from:

Hibernate Search comes with a range of pluggable Elasticsearch REST clients that you can pick from:

or

Suggested change

Hibernate Search comes with a range of pluggable Elasticsearch REST clients that the user can pick from:

Hibernate Search comes with a range of pluggable Elasticsearch REST clients that applications can pick from:

yrodiere · 2025-10-23T14:20:29Z

documentation/src/main/asciidoc/migration/index.adoc

Maybe add something to the "Artifacts" section, mentioning that applications should explicitly depend on the client they want to use, if they do care about which client is used?

since the behavior of this client is slightly different

…plement the clients as dependencies

…he backend and remove the client-common

…ackend

sonarqubecloud · 2025-10-29T16:19:36Z

Quality Gate failed

Failed conditions
71.4% Coverage on New Code (required ≥ 80%)
18.6% Duplication on New Code (required ≤ 3%)

See analysis details on SonarQube Cloud

marko-bekhta force-pushed the feat/HSEARCH-5464-Groundwork-for-pluggable-rest-clients-in-the-Elasticsearch-backend branch 7 times, most recently from aac26ce to 1d41060 Compare September 10, 2025 14:31

marko-bekhta force-pushed the feat/HSEARCH-5464-Groundwork-for-pluggable-rest-clients-in-the-Elasticsearch-backend branch 3 times, most recently from 3da3726 to 610c554 Compare September 16, 2025 21:10

marko-bekhta force-pushed the feat/HSEARCH-5464-Groundwork-for-pluggable-rest-clients-in-the-Elasticsearch-backend branch 3 times, most recently from e91fd98 to ae10272 Compare October 16, 2025 08:29

marko-bekhta commented Oct 16, 2025

View reviewed changes

marko-bekhta force-pushed the feat/HSEARCH-5464-Groundwork-for-pluggable-rest-clients-in-the-Elasticsearch-backend branch from e347a5f to 2e6dca2 Compare October 16, 2025 20:30

yrodiere reviewed Oct 23, 2025

View reviewed changes

marko-bekhta added 11 commits October 24, 2025 13:58

HSEARCH-5464 Remove the apache host from ElasticsearchResponse

58e96b1

HSEARCH-5464 Move out client common classes

0484698

HSEARCH-5464 Remove dependency on ES version class in the client code

8438a68

HSEARCH-5464 Continue factoring out the Client

756dd0a

HSEARCH-5464 OpenSearch client

f74581d

HSEARCH-5464 Elasticsearch-java client

27541fe

HSEARCH-5464 Test different clients

21cf9b4

HSEARCH-5464 Rename the clients

e5d9e5e

HSEARCH-5464 Adjust elasticsearch-java tests

c141536

since the behavior of this client is slightly different

HSEARCH-5464 Fix aggregated javadocs and dependency alignment

e33dbee

HSEARCH-5464 More renames to address JQAssistant rules

4f36c56

marko-bekhta added 12 commits October 24, 2025 13:59

HSEARCH-5464 Explicitly list the apache http client libs we use to im…

ec5c0e5

…plement the clients as dependencies

HSEARCH-5464 Push client specific settings to client modules

16f0121

HSEARCH-5464 Make AWS signing compatible with other client impls

393736b

HSEARCH-5464 Adjust search-util-common exports

73c067a

HSEARCH-5464 Add missing platform dependencies

a3e54be

HSEARCH-5464 Add a few notes on the new pluggable clients

b83183c

HSEARCH-5464 Move (most of) the elasticsearch-client-rest back into t…

1d0d3bf

…he backend and remove the client-common

HSEARCH-5464 Address some of the review comments

98bbb78

HSEARCH-5464 Switch to the new elasticsearch-rest5-client

4eb86b4

HSEARCH-5464 Rename Elasticsearch clients and their packages

68c6fab

HSEARCH-5464 Clarify a few points in the docs

1ba07a2

HSEARCH-5464 Mark rest5 and opensearch clients as incubating

8a4937a

marko-bekhta force-pushed the feat/HSEARCH-5464-Groundwork-for-pluggable-rest-clients-in-the-Elasticsearch-backend branch 7 times, most recently from 12e8879 to d4e3fce Compare October 25, 2025 20:26

HSEARCH-5464 Finish with renaming and JQAssistant

b805101

marko-bekhta force-pushed the feat/HSEARCH-5464-Groundwork-for-pluggable-rest-clients-in-the-Elasticsearch-backend branch from d4e3fce to b805101 Compare October 25, 2025 21:07

HSEARCH-5464 Move around gson entity to keep the common code in the b…

3152541

…ackend

	@Message(id = ID_OFFSET_LEGACY_ENGINE + 52,
	value = "An index writer operation failed. Resetting the index writer and forcing release of locks. %1$s")
	void indexWriterResetAfterFailure(@FormatWith(EventContextFormatter.class) EventContext context);

	@Message(id = ID_OFFSET + 16,
	value = "Invalid tenant identifiers: '%1$s'."
	+ " No tenant identifier is expected, because multi-tenancy is disabled for this backend.")
	SearchException tenantIdProvidedButMultiTenancyDisabled(Set<String> tenantIds, @Param EventContext context);

	But, there are some deprecations:

	However, some elements are now deprecated:

	Hibernate Search comes with a range of pluggable Elasticsearch REST clients that the user can pick from:
	Hibernate Search comes with a range of pluggable Elasticsearch REST clients that you can pick from:

Uh oh!

HSEARCH-5464 Pluggable rest clients in the elasticsearch backend #4798

Are you sure you want to change the base?

HSEARCH-5464 Pluggable rest clients in the elasticsearch backend #4798

Uh oh!

Conversation

marko-bekhta commented Sep 9, 2025

Uh oh!

marko-bekhta left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

yrodiere left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

marko-bekhta commented Oct 23, 2025

Uh oh!

yrodiere commented Oct 23, 2025

Uh oh!

yrodiere left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sonarqubecloud bot commented Oct 29, 2025

Quality Gate failed

Uh oh!

Reviewers

Assignees