Introduce extensions for the Elasticsearch clients #10745

gsmet · 2020-07-15T11:46:11Z

@loicmathieu this is a continuation of your PR here #8290 with cleaner commits and some additional changes:

I fixed a few doc issues
I used the Feature class for the features
I cleaned up the package names for more consistency. Keep in mind that non-API runtime classes should be in the runtime subpackage
I added a description for the visible runtime extensions as it will be visible at code.quarkus.io.
I changed the bean initialization a bit by avoiding the @PostConstruct pattern.

All in all, its all minor changes but I wanted to make a list so that you have them in mind when reviewing (and for the next extensions).

Could you have a look and check everything is OK with you?

I preferred opening another PR so that we still have yours handy in case you want to compare or I missed something important.

gsmet · 2020-07-15T11:46:50Z

@yrodiere I think it's time for us to get your opinion on this. Thanks!

yrodiere

I added a few comments here and there regarding missing features or potential bugs, but it looks good overall.

If we want the Hibernate Search Elasticsearch extension to use the same REST client, I believe we'll have to handle multi-client configurations first. Right now only one client can be defined, but Hibernate Search allows multiple backends, each using a different client targeting a different cluster.
Once we have that, I'll have to change Hibernate Search to allow Quarkus to provide its own client. I'd be in favor of moving all the client configuration over to your Elasticsearch REST client extension, and removing it from the Hibernate Search extension; that way, configuration would be more similar to Hibernate ORM and its datasources, and I believe it would be more true to Quarkus' "opinionated" spirit.

There's one more blocker for Hibernate Search to use this extension, though: AWS support. People who want to connect to AWS Elasticsearch Service need to sign their requests, which means they need to plug in custom request processors, which means they need access to the HTTP client builder at some point.
In Hibernate Search, we provide a dedicated extension that relies on an internal extension point. You'll probably need something similar (at least the extension point) if you want to allow people to use this client with AWS Elasticsearch Service. If you decide to go a step further and provide built-in support for AWS request signing, let's discuss it first, as I plan to rework what we do in Hibernate Search, too.

.../src/main/java/io/quarkus/elasticsearch/restclient/lowlevel/runtime/ElasticsearchConfig.java

yrodiere · 2020-07-15T11:58:23Z

.../main/java/io/quarkus/elasticsearch/restclient/lowlevel/runtime/RestClientBuilderHelper.java

+
+    static RestClientBuilder createRestClientBuilder(ElasticsearchConfig config) {
+        List<HttpHost> hosts = config.hosts.stream().map(s -> new HttpHost(s.substring(0, s.indexOf(":")),
+                Integer.parseInt(s.substring(s.indexOf(":") + 1)), config.protocol)).collect(Collectors.toList());


Shouldn't the port be optional and default to 9200?

I'm not sure tbh but we should at least change the config stuff to use InetSocketAddress IMHO.

mongodb, hibernate-search-elasticsearch and neo4j uses such a construct so I think it's OK here.

We can of course make the port optional but it's really standard to define elasticsearch URL this way so I wouldn't bother as it will implies a lot more branches in the configuration code.

yrodiere · 2020-07-15T12:00:07Z

.../main/java/io/quarkus/elasticsearch/restclient/lowlevel/runtime/RestClientBuilderHelper.java

+        builder.setHttpClientConfigCallback(new RestClientBuilder.HttpClientConfigCallback() {
+            @Override
+            public HttpAsyncClientBuilder customizeHttpClient(HttpAsyncClientBuilder httpClientBuilder) {
+                if (config.username.isPresent()) {


In Hibernate Search, we issue a warning in this case if the protocol is set to http (the default), because then the password will be transmitted using a clear-text connection, which is a security issue on public networks.

That's a good idea.

yrodiere · 2020-07-15T12:39:28Z

.../src/main/java/io/quarkus/elasticsearch/restclient/lowlevel/runtime/ElasticsearchConfig.java

+     */
+    @ConfigItem
+    public Optional<Integer> ioThreadCounts;
+


Connections settings, which I think are quite important, are missing:

max_connections: the maximum number of connections to the cluster.

max_connections_per_route: the maximum number of connections to each node in the cluster.

See https://github.com/hibernate/hibernate-search/blob/9da99d7de8f346aa0ec81ae2ce0a1fa2c1657b2a/backend/elasticsearch/src/main/java/org/hibernate/search/backend/elasticsearch/client/impl/ElasticsearchClientFactoryImpl.java#L201-L200

The documentation now only references this one. See https://www.elastic.co/guide/en/elasticsearch/client/java-rest/current/_number_of_threads.html

I think it's because of https://www.elastic.co/guide/en/elasticsearch/client/java-rest/current/_others.html . They basically send you to the Apache HTTP client doc.

I would trust Yoann on that one :).

I'm not sure I'm following. The settings I mentioned are settings of the underlying HTTP client, not of the Elasticsearch Rest client. So indeed, they don't have dedicated entries in the Elasticsearch Rest client documentation. They are still relevant, though. See https://www.elastic.co/guide/en/elasticsearch/client/java-rest/current/_others.html

Yeah, I understand it now, it's different things ;)

yrodiere · 2020-07-15T12:41:18Z

.../main/java/io/quarkus/elasticsearch/restclient/lowlevel/runtime/RestClientBuilderHelper.java

+                    IOReactorConfig ioReactorConfig = IOReactorConfig.custom()
+                            .setIoThreadCount(config.ioThreadCounts.get())
+                            .build();
+                    httpClientBuilder.setDefaultIOReactorConfig(ioReactorConfig);


You may want to customize the thread factory to give a better name to client threads (something that mentions Elasticsearch).

See https://github.com/hibernate/hibernate-search/blob/9da99d7de8f346aa0ec81ae2ce0a1fa2c1657b2a/backend/elasticsearch/src/main/java/org/hibernate/search/backend/elasticsearch/client/impl/ElasticsearchClientFactoryImpl.java#L203

We don't have a ThreadProvider available yet inside the extension so I would not implement this now.

Right, that's something specific to Hibernate Search. You can implement a much simpler ThreadFactory that simply creates threads with an appropriate name. For example like this: https://github.com/hibernate/hibernate-search/blob/9da99d7de8f346aa0ec81ae2ce0a1fa2c1657b2a/legacy/engine/src/main/java/org/hibernate/search/util/impl/SearchThreadFactory.java

That is, if you consider thread names are important.

yrodiere · 2020-07-15T12:42:48Z

.../main/java/io/quarkus/elasticsearch/restclient/lowlevel/runtime/RestClientBuilderHelper.java

+                            .build();
+                    httpClientBuilder.setDefaultIOReactorConfig(ioReactorConfig);
+                }
+


You don't seem to be using the "protocol" setting. You should use it here to disable SSL if it's not necessary, because SSL causes significant slowdowns on startup.

See https://github.com/hibernate/hibernate-search/blob/9da99d7de8f346aa0ec81ae2ce0a1fa2c1657b2a/backend/elasticsearch/src/main/java/org/hibernate/search/backend/elasticsearch/client/impl/ElasticsearchClientFactoryImpl.java#L204-L208

yrodiere · 2020-07-15T12:43:58Z

.../main/java/io/quarkus/elasticsearch/restclient/lowlevel/runtime/RestClientBuilderHelper.java

+    static Sniffer createSniffer(RestClient client, ElasticsearchConfig config) {
+        return Sniffer.builder(client)
+                .setSniffIntervalMillis((int) config.discovery.refreshInterval.toMillis())
+                .build();


You need to take the protocol into account here, too.

See https://github.com/hibernate/hibernate-search/blob/9da99d7de8f346aa0ec81ae2ce0a1fa2c1657b2a/backend/elasticsearch/src/main/java/org/hibernate/search/backend/elasticsearch/client/impl/ElasticsearchClientFactoryImpl.java#L182-L190

yrodiere · 2020-07-15T12:47:20Z

.../main/java/io/quarkus/elasticsearch/restclient/lowlevel/runtime/RestClientBuilderHelper.java

+            public RequestConfig.Builder customizeRequestConfig(RequestConfig.Builder requestConfigBuilder) {
+                return requestConfigBuilder
+                        .setConnectTimeout((int) config.connectionTimeout.toMillis())
+                        .setSocketTimeout((int) config.socketTimeout.toMillis());


Not sure it's still relevant nowadays, but you might want to call setConnectionRequestTimeout( 0 ) here to avoid requests being flagged as timed out even when they didn't time out.

See https://hibernate.atlassian.net/browse/HSEARCH-2681
See https://github.com/hibernate/hibernate-search/blob/9da99d7de8f346aa0ec81ae2ce0a1fa2c1657b2a/backend/elasticsearch/src/main/java/org/hibernate/search/backend/elasticsearch/client/impl/ElasticsearchClientFactoryImpl.java#L236

loicmathieu

LGTM, I added some minor comments but as it's mainly what I implemented in my original PR aprove it ;)

To answer you bullet list :

I fixed a few doc issues

As always ;) thanks for this!

I used the Feature class for the features

See my comment, I think it was disadvised now as it is complex to maintain and don't provides any advantage.

I cleaned up the package names for more consistency. Keep in mind that non-API runtime classes should be in the runtime subpackage

Yep, thanks

I added a description for the visible runtime extensions as it will be visible at code.quarkus.io.

I always forgot about this! Thanks

I changed the bean initialization a bit by avoiding the @PostConstruct pattern.

In the begining, I didn't use the Singleton scope so I eagerly create the client in a PostConstruct method. Now that is uses Singleton it's more simple like this.

loicmathieu · 2020-07-15T12:32:37Z

core/deployment/src/main/java/io/quarkus/deployment/Feature.java

+    ELASTICSEARCH_REST_CLIENT_COMMON,
    ELASTICSEARCH_REST_CLIENT,
+    ELASTICSEARCH_REST_HIGH_LEVEL_CLIENT,


I thought enumeration of the features inside th Feature class was something we want to avoid for now.
I remember some discussion about this last time I created an extension ...

Yeah, well, no :).

I'm pretty sure Alexey or David didn't agree with you but OK :)

extensions/elasticsearch-rest-client/runtime/pom.xml

...kus/elasticsearch/restclient/highlevel/runtime/ElasticsearchRestHighLevelClientProducer.java

gsmet · 2020-07-15T14:45:56Z

@loicmathieu some of Yoann's point can be dealt with in a followup but I think some of the issues need to be addressed before merging, especially the potential injection issue, the SSL not being fully included the password.get() and a few others.

The more cosmetic ones, I would say we can merge a first iteration and then build on it.

It would be nice if you could use fixup commits attached to the appropriate commit with git commit --fixup <sha> and then I will autosquash at the end.

It should be pretty easy as the commits are well split.

gsmet · 2020-07-15T14:47:48Z

@loicmathieu I pushed the easy suggestions from Yoann, I let you play add the fixup commits to my branch? You should be authorized to. Ping me if not!

loicmathieu · 2020-07-15T14:50:55Z

@loicmathieu some of Yoann's point can be dealt with in a followup but I think some of the issues need to be addressed before merging, especially the potential injection issue, the SSL not being fully included the password.get() and a few others.

OK, will do it. For the injection issue this is quickstart code is it really important ? I can use Vert.x JsonObject I think it's available everywhere but it will need Jackson. Or I can juts add a comment // never do this as it is a query injection ....

gsmet · 2020-07-15T15:03:03Z

AFAIK you are already using Jackson in the doc so that would be fine.

yrodiere · 2020-07-21T15:56:57Z

.../src/main/java/io/quarkus/elasticsearch/restclient/lowlevel/runtime/ElasticsearchConfig.java

@@ -16,7 +17,7 @@
     * The list of hosts of the Elasticsearch servers.
     */
    @ConfigItem(defaultValue = "localhost:9200")
-    public List<String> hosts;
+    public List<InetSocketAddress> hosts;


From the javadoc:

* This class implements an IP Socket Address (IP address + port number) * It can also be a pair (hostname + port number), in which case an attempt * will be made to resolve the hostname.

Do you really want that?

We use InetSocketAddress in Quarkus for all the host:port combinations. I think, if I use getHostString() as I did, it does not resolve anything. At least that's what the Javadoc says.

I checked (statically) what java.net.InetSocketAddress#InetSocketAddress(java.lang.String, int) calls and ended up in java.net.InetAddress#getAddressesFromNameService... Might be worth a double-check.

It will be used by both the Hibernate Search - Elasticsearch extension and the Elasticsearch client extensions. Co-authored-by: Guillaume Smet <[email protected]>

Co-authored-by: Guillaume Smet <[email protected]>

gsmet · 2020-07-22T07:36:50Z

Merged, thanks everyone!

boring-cyborg bot added area/core area/dependencies Pull requests that update a dependency file area/documentation area/hibernate-search Hibernate Search area/infra-automation anything related to CI, bots, etc. that are used to automated our infrastructure labels Jul 15, 2020

gsmet requested a review from loicmathieu July 15, 2020 11:46

gsmet added this to the 1.7.0 - master milestone Jul 15, 2020

yrodiere reviewed Jul 15, 2020

View reviewed changes

loicmathieu approved these changes Jul 15, 2020

View reviewed changes

gsmet force-pushed the elasticsearch-clients-gsmet branch from c02eb2b to c107b74 Compare July 15, 2020 14:40

gsmet force-pushed the elasticsearch-clients-gsmet branch from d19542b to 3484146 Compare July 21, 2020 15:33

yrodiere reviewed Jul 21, 2020

View reviewed changes

gsmet mentioned this pull request Jul 21, 2020

Elasticsearch low and high level REST clients #8290

Closed

gsmet force-pushed the elasticsearch-clients-gsmet branch from dfbccaa to 5243831 Compare July 21, 2020 16:14

loicmathieu and others added 5 commits July 21, 2020 18:16

Rename elasticsearch-rest-client extension to -common

f669a26

It will be used by both the Hibernate Search - Elasticsearch extension and the Elasticsearch client extensions. Co-authored-by: Guillaume Smet <[email protected]>

Add an Elasticsearch low level client extension

85643a2

Co-authored-by: Guillaume Smet <[email protected]>

Add an Elasticsearch high level client extension

183f04e

Co-authored-by: Guillaume Smet <[email protected]>

Add documentation for the new Elasticsearch client extensions

95c51da

Co-authored-by: Guillaume Smet <[email protected]>

Move the Elasticsearch native ITs to a specific Data6 job

ecf34a6

gsmet force-pushed the elasticsearch-clients-gsmet branch from 5243831 to ecf34a6 Compare July 21, 2020 16:17

gsmet merged commit 498f1f0 into quarkusio:master Jul 22, 2020

gsmet added the release/noteworthy-feature label Aug 13, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introduce extensions for the Elasticsearch clients #10745

Introduce extensions for the Elasticsearch clients #10745

gsmet commented Jul 15, 2020

gsmet commented Jul 15, 2020

yrodiere left a comment

yrodiere Jul 15, 2020

gsmet Jul 15, 2020

loicmathieu Jul 15, 2020

yrodiere Jul 15, 2020

gsmet Jul 15, 2020

yrodiere Jul 15, 2020 •

edited

Loading

loicmathieu Jul 15, 2020

gsmet Jul 15, 2020

yrodiere Jul 15, 2020

loicmathieu Jul 15, 2020

yrodiere Jul 15, 2020

loicmathieu Jul 16, 2020

yrodiere Jul 16, 2020

yrodiere Jul 15, 2020

yrodiere Jul 15, 2020

yrodiere Jul 15, 2020 •

edited

Loading

loicmathieu left a comment

loicmathieu Jul 15, 2020

gsmet Jul 15, 2020

loicmathieu Jul 15, 2020

gsmet commented Jul 15, 2020

gsmet commented Jul 15, 2020

loicmathieu commented Jul 15, 2020

gsmet commented Jul 15, 2020

yrodiere Jul 21, 2020

gsmet Jul 21, 2020

yrodiere Jul 21, 2020

gsmet commented Jul 22, 2020

Introduce extensions for the Elasticsearch clients #10745

Introduce extensions for the Elasticsearch clients #10745

Conversation

gsmet commented Jul 15, 2020

gsmet commented Jul 15, 2020

yrodiere left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yrodiere Jul 15, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yrodiere Jul 15, 2020 • edited Loading

Choose a reason for hiding this comment

loicmathieu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gsmet commented Jul 15, 2020

gsmet commented Jul 15, 2020

loicmathieu commented Jul 15, 2020

gsmet commented Jul 15, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gsmet commented Jul 22, 2020

yrodiere Jul 15, 2020 •

edited

Loading

yrodiere Jul 15, 2020 •

edited

Loading