-
Notifications
You must be signed in to change notification settings - Fork 2.7k
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
Add a way to configure an OpenSearch distribution for the Elasticsearch DevService #34958
Add a way to configure an OpenSearch distribution for the Elasticsearch DevService #34958
Conversation
/cc @gsmet (elasticsearch,hibernate-search), @loicmathieu (elasticsearch), @yrodiere (elasticsearch,hibernate-search) |
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.
Spotted a few minor things, otherwise that's great (but I will probably let Yoann have a look when he's back).
As for versions being a bit weird, it all boils down to us still supporting the old Elasticsearch high level client, but we should fix this soon hopefully: see #31997 .
...io/quarkus/elasticsearch/restclient/common/deployment/DevServicesElasticsearchProcessor.java
Outdated
Show resolved
Hide resolved
...rkus/elasticsearch/restclient/common/deployment/ElasticsearchDevServicesBuildTimeConfig.java
Outdated
Show resolved
Hide resolved
This comment has been minimized.
This comment has been minimized.
Thanks for taking a look and for the link! 😃 I've addressed your suggestions, but will keep them locally for now, to not trigger more builds for now 😃
yeah, I've also noticed the comments that we can only start the default backend, but couldn't think of anything obvious that would limit us from starting multiple instances, so wanted to check with Yoann on that too once he's back 😃 |
See #24011, which has a few links that might explain the history of this limitation. I don't know from the top of my head why it's there. Maybe it was just the minimum viable product, and we never went beyond that. |
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.
Thanks. I agree with Guillaume's comments, and added some of my own below.
...rkus/elasticsearch/restclient/common/deployment/ElasticsearchDevServicesBuildTimeConfig.java
Outdated
Show resolved
Hide resolved
integration-tests/hibernate-search-orm-opensearch/src/main/resources/application.properties
Outdated
Show resolved
Hide resolved
...orm/opensearch/devservices/HibernateSearchElasticsearchDevServicesEnabledImplicitlyTest.java
Outdated
Show resolved
Hide resolved
17828c7
to
99a3ed5
Compare
🙈 The PR is closed and the preview is expired. |
This comment has been minimized.
This comment has been minimized.
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.
Thanks! The automatic resolution of Elasticsearch/OpenSearch will be a great addition.
A few comments on the implementation below.
...rkus/elasticsearch/restclient/common/deployment/ElasticsearchDevServicesBuildTimeConfig.java
Outdated
Show resolved
Hide resolved
...rkus/elasticsearch/restclient/common/deployment/ElasticsearchDevServicesBuildTimeConfig.java
Outdated
Show resolved
Hide resolved
...io/quarkus/elasticsearch/restclient/common/deployment/DevServicesElasticsearchProcessor.java
Outdated
Show resolved
Hide resolved
...io/quarkus/elasticsearch/restclient/common/deployment/DevServicesElasticsearchProcessor.java
Outdated
Show resolved
Hide resolved
...io/quarkus/elasticsearch/restclient/common/deployment/DevServicesElasticsearchProcessor.java
Outdated
Show resolved
Hide resolved
...io/quarkus/elasticsearch/restclient/common/deployment/DevServicesElasticsearchProcessor.java
Outdated
Show resolved
Hide resolved
...io/quarkus/elasticsearch/restclient/common/deployment/DevServicesElasticsearchProcessor.java
Outdated
Show resolved
Hide resolved
99a3ed5
to
c4005b5
Compare
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, thanks!
This comment has been minimized.
This comment has been minimized.
c4005b5
to
084f57f
Compare
This comment has been minimized.
This comment has been minimized.
Ok that's a new one:
I'll don't see how that would be caused by this PR though... I'll ask around. |
084f57f
to
cd90a56
Compare
I rebased to get at least one JVM build green. |
Fixes #23906
This was among the things Yoann suggested to look into. While doing so, I noticed that the guide had property placeholders instead of values -- fixed that.
Also, I wanted to better understand the logic behind the used Elasticsearch/OpenSearch image versions, but couldn't figure it out 😃 so any pointers are appreciated.
elasticsearch-server.version
version that we have in POM (7.16.3
) and is used for testing doesn't match the default we have in the config for dev services (7.17.0
) and the ES client is8.8.2
(while7.10.2
for proprietary components, which is expected). As for OpenSearch,opensearch-server.version
is1.2.3
while there's already a 2.8.0. At least, I'd expect that the version we are testing against and the version of dev services would match...