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.

Sign up for GitHub

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

Jump to bottom

Update Hibernate Search dev console card for multiple persistence-units #25578

Merged
merged 1 commit into from
May 24, 2022
Merged

Update Hibernate Search dev console card for multiple persistence-units #25578

merged 1 commit into from
May 24, 2022

Conversation

mun711
Copy link
Contributor

@mun711 mun711 commented May 14, 2022 •
edited by yrodiere

Fixes #14729

@quarkus-bot quarkus-bot bot added the area/hibernate-search Hibernate Search / Elasticsearch label May 14, 2022
@quarkus-bot
Copy link

quarkus-bot bot commented May 14, 2022

Thanks for your pull request!

The title of your pull request does not follow our editorial rules. Could you have a look?

  • title should preferably start with an uppercase character (if it makes sense!)

This message is automatically generated by a bot.

@mun711 mun711 changed the title [WIP] Update Hibernate Search dev console card for multiple persistence-units WIP - Update Hibernate Search dev console card for multiple persistence-units May 14, 2022
@mun711 mun711 mentioned this pull request May 14, 2022
Closed
@Sanne Sanne requested a review from yrodiere May 14, 2022 23:04
@mun711 mun711 force-pushed the hibernate-search-dev-console-multi-pu branch from 766109d to 03efce8 Compare May 15, 2022 20:56
yrodiere
yrodiere requested changes May 16, 2022
View reviewed changes
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.

Thanks! I added a few comments below.

Regarding your statements on the issue:

Didn't add multiple hibernate search cards per persistence unit, but rather added multiple tables on the page referenced by the card (it seems that this kind of grouping is used for other cards that display some multi pu data)

Agreed, that's a better solution than multiple cards.

Not sure if we can have entities with same name across multiple persistence units

Yes we can.

regarding the indexing, it will check and reindex the selected classes if found in the search mapping using the jpa entity name, should I add an explicit expectation for the pu or use the full class name?

Using the class name may not solve the problem; in general you can use the same entity class in multiple persistence units, though I'm not sure that's possible in Quarkus.

It's probably safer to be explicit about the targeted persistence unit; see my comment below.

...hibernate-search-orm-elasticsearch/deployment/src/main/resources/dev-templates/embedded.html Show resolved Hide resolved
...rnate-search-orm-elasticsearch/deployment/src/main/resources/dev-templates/entity-types.html
@@ -1,11 +1,14 @@
{#include main}
{#title}Indexed Entities{/title}
{#body}
{#if info:indexedEntityTypes.isEmpty}
{#if info:indexedEntityTypes.isEmpty || info:indexedEntityTypes.allValuesEmpty}
<p>No indexed entities were found.</p>
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
<p>No indexed entities were found.</p>
<p>No persistence units were found.</p>

...o/quarkus/hibernate/search/orm/elasticsearch/runtime/devconsole/HibernateSearchSupplier.java Outdated Show resolved Hide resolved
independent-projects/qute/core/src/main/java/io/quarkus/qute/ValueResolvers.java Outdated Show resolved Hide resolved
...hibernate/search/orm/elasticsearch/runtime/devconsole/HibernateSearchDevConsoleRecorder.java Outdated Show resolved Hide resolved
...o/quarkus/hibernate/search/orm/elasticsearch/runtime/devconsole/HibernateSearchSupplier.java Outdated Show resolved Hide resolved
...o/quarkus/hibernate/search/orm/elasticsearch/runtime/devconsole/HibernateSearchSupplier.java Outdated Show resolved Hide resolved
@mun711 mun711 force-pushed the hibernate-search-dev-console-multi-pu branch from 03efce8 to 7dc58da Compare May 19, 2022 17:25
@mun711 mun711 force-pushed the hibernate-search-dev-console-multi-pu branch from 7dc58da to 200557f Compare May 19, 2022 18:05
@mun711 mun711 requested a review from yrodiere May 20, 2022 11:23
@mun711 mun711 force-pushed the hibernate-search-dev-console-multi-pu branch from 200557f to 61d2ee6 Compare May 21, 2022 16:25
yrodiere
yrodiere requested changes May 23, 2022
View reviewed changes
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.

Thanks. There's a bug when multiple entities from the same persistence unit are selected for reindexing, but apart from that it looks good. Let's merged once you fixed that :)

...o/quarkus/hibernate/search/orm/elasticsearch/runtime/devconsole/HibernateSearchSupplier.java Outdated Show resolved Hide resolved
...rnate-search-orm-elasticsearch/deployment/src/main/resources/dev-templates/entity-types.html
Comment on lines +34 to +35
<input type="checkbox" class="form-check-input checkbox" name="{indexedEntityType.jpaName}"
id="{indexedEntityType.jpaName}" value="{indexedPersistenceUnit.persistenceUnitName}">
Copy link
Member

Choose a reason for hiding this comment

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

👍 I checked, this works even if there are multiple entities in different PUs with the same name.

...hibernate/search/orm/elasticsearch/runtime/devconsole/HibernateSearchDevConsoleRecorder.java Outdated Show resolved Hide resolved
@mun711 mun711 force-pushed the hibernate-search-dev-console-multi-pu branch from 61d2ee6 to 1b3550f Compare May 23, 2022 10:56
@yrodiere yrodiere changed the title WIP - Update Hibernate Search dev console card for multiple persistence-units Update Hibernate Search dev console card for multiple persistence-units May 23, 2022
@yrodiere yrodiere added the triage/waiting-for-ci Ready to merge when CI successfully finishes label May 23, 2022
@yrodiere
Copy link
Member

This looks good, thanks! Let's merge once CI passes.

@mun711 I don't think the problem you noticed when adding @Indexed is related to your work; would you mind filing a separate issue, please?

@yrodiere yrodiere marked this pull request as draft May 23, 2022 13:17
@yrodiere yrodiere marked this pull request as ready for review May 23, 2022 13:17
@mun711
Copy link
Contributor Author

mun711 commented May 23, 2022

@mun711 I don't think the problem you noticed when adding @Indexed is related to your work; would you mind filing a separate issue, please?

Sure, will check out more details and open an issue in the next few days

@yrodiere yrodiere merged commit 1963eee into quarkusio:main May 24, 2022
@quarkus-bot quarkus-bot bot added this to the 2.10 - main milestone May 24, 2022
@quarkus-bot quarkus-bot bot added kind/enhancement New feature or request and removed triage/waiting-for-ci Ready to merge when CI successfully finishes labels May 24, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@yrodiere yrodiere yrodiere approved these changes

Assignees
No one assigned
Labels
area/hibernate-orm Hibernate ORM area/hibernate-search Hibernate Search / Elasticsearch area/persistence kind/enhancement New feature or request
Projects
None yet
Milestone
2.10.0.CR1
Development

Successfully merging this pull request may close these issues.

Hibernate Search dev console card for multi-persistence-unit applications
2 participants
@mun711 @yrodiere