-
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
Shared extension for the Hibernate Search Elasticsearch backend #39416
base: main
Are you sure you want to change the base?
Conversation
/cc @gsmet (hibernate-search) |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
eab51da
to
9ecb4e8
Compare
9ecb4e8
to
dcbbe40
Compare
Rebased on main. Ran a few tests locally, and it seems to be working fine... we'll need to check the generated documentation for configuration properties though. |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
dcbbe40
to
1d3bb33
Compare
This comment has been minimized.
This comment has been minimized.
fb9e99c
to
5bff5be
Compare
This comment has been minimized.
This comment has been minimized.
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.
This is a nice improvement! 😃
There are a couple of failing tests because of the missing pu-config, I'll let you decide how you want to address those 😃. Other than that, it looks good!
...in/java/io/quarkus/hibernate/search/orm/elasticsearch/runtime/HibernateSearchConfigUtil.java
Show resolved
Hide resolved
...e/search/backend/elasticsearch/runtime/HibernateSearchBackendElasticsearchRuntimeConfig.java
Outdated
Show resolved
Hide resolved
...te/search/backend/elasticsearch/deployment/HibernateSearchBackendElasticsearchProcessor.java
Outdated
Show resolved
Hide resolved
...kus/hibernate/search/orm/elasticsearch/deployment/HibernateSearchElasticsearchProcessor.java
Show resolved
Hide resolved
5bff5be
to
44d8359
Compare
2c79e08
to
e5a7335
Compare
This comment has been minimized.
This comment has been minimized.
🎊 PR Preview 7ab2138 has been successfully built and deployed to https://quarkus-pr-main-39416-preview.surge.sh/version/main/guides/
|
This comment has been minimized.
This comment has been minimized.
e5a7335
to
fb98664
Compare
#43943 was merged, I rebased on main, so I think we're all set. @marko-bekhta I think I addressed your comments, do you mind approving please? |
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.
🎉
I'll have a look later today. |
This comment has been minimized.
This comment has been minimized.
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.
Sorry for the very late review. I added only one (annoying, sorry...) comment. I think it would be worth taking it into account even if I know you won't be very excited about it :).
extensions/hibernate-search-backend-elasticsearch/deployment/pom.xml
Outdated
Show resolved
Hide resolved
fb98664
to
1c43f90
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.
I renamed the extension to quarkus-hibernate-search-backend-elasticsearch-common
as requested.
Status for workflow
|
Status for workflow
|
To reduce code duplication.
Opening as draft, because:It's based on Refresh documentation (and some tests) of the Hibernate Search + ORM extension #39413 and Extension for the Hibernate Search Standalone Pojo Mapper with Elasticsearch #39415, which must be merged first.=> DoneIt requires Rework how configuration doc generation works #35439 to get fixed first, and that will take a while; see https://quarkusio.zulipchat.com/#narrow/stream/187038-dev/topic/Reusing.20ConfigMapping.20from.20other.20JARS=> DoneBased on #43943, which must be merged first.=> Done