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.

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

WFLY-15838 + WFLY-15839 + WFLY-15840 + WFLY-15841 + WFLY-15842 Upgrade to Hibernate Search 6 (EE 9 + ORM 6) #15185

Merged
merged 18 commits into from May 12, 2022

Conversation

yrodiere
Copy link
Contributor

@yrodiere yrodiere commented Feb 3, 2022

  • WFLY-15842: Add (private) module for Elasticsearch's low-level REST client
  • WFLY-15841: Add (private) module for com.carrotsearch.hppc
  • WFLY-15840: Upgrade to Apache Avro 1.11.0
  • WFLY-15839: Upgrade to Apache Lucene 8.11
  • WFLY-15838: Upgrade to Hibernate Search 6.1

Based on #15174 (WFLY-15992) and #15165 (WFLY-15440), which need to be merged first. => Done

Supersedes #15031.

@github-actions
Copy link

github-actions bot commented Feb 3, 2022

Dependency Tree Analyzer Output:

New Dependencies:

  • com.carrotsearch:hppc:jar:0.8.1:compile
  • org.apache.lucene:lucene-join:jar:5.5.5:compile
  • org.elasticsearch.client:elasticsearch-rest-client-sniffer:jar:7.16.3:compile
  • org.elasticsearch.client:elasticsearch-rest-client:jar:7.16.3:compile

Removed Dependencies:

  • org.hibernate:hibernate-search-backend-jms:jar:5.10.11.Final:compile
  • org.hibernate:hibernate-search-engine:jar:5.10.11.Final:compile
  • org.hibernate:hibernate-search-orm:jar:5.10.11.Final:compile
  • org.hibernate:hibernate-search-serialization-avro:jar:5.10.11.Final:compile

CC @wildfly/prod

@github-actions github-actions bot added the deps-changed Dependencies have been checked, and there are changes highlighted in a comment label Feb 3, 2022
@yrodiere
Copy link
Contributor Author

yrodiere commented Feb 3, 2022

As far as I can see the failure in Linux - JDK 11 is the one about compat tests, which Scott mentioned in #15165. I didn't look at the other failures.

I ran the tests related to Hibernate Search locally though, and I found one failure. It's caused by a bug in Hibernate ORM 6: hibernate/hibernate-orm#4777

@scottmarlow
Copy link
Contributor

@yrodiere fyi, https://github.com/scottmarlow/wildfly/tree/orm6_fixup2 has more updates to pass more tests (especially if you use the latest ORM 6.0 snapshot build).

@yrodiere
Copy link
Contributor Author

@scottmarlow Thanks for the heads-up, I just rebased. Let's see how it goes...

@scottmarlow
Copy link
Contributor

@yrodiere I think that ORM 6 (in JPA/Persistence compatible mode) needs queries to include a SELECT alias. I updated some other tests from:

query = em.createQuery("from Employee e where e.id=:id");

To:

query = em.createQuery("select e from Employee e where e.id=:id");

And from:

query = em.createQuery("from Employee");

To:

query = em.createQuery("select e from Employee e");

And from:

        {@NamedQuery(name = "allCustomers", query = "from Customer"),
                @NamedQuery(name = "customerById", query = "from Customer c where c.id=:id")})
 public class Customer {

To:

       {@NamedQuery(name = "allCustomers", query = "select c from Customer c"),
               @NamedQuery(name = "customerById", query = "select c from Customer c where c.id=:id")})
 public class Customer {

@scottmarlow
Copy link
Contributor

scottmarlow commented Mar 28, 2022

If you see failures due to the CDI Bean Manager not being available, that is likely caused by a change to not enable CDI by default.

jakartaee/platform-tck#874 mentions you can workaround that a few ways like specifying that beans are Application scoped (and a few other ways mentioned in the pr).

@yrodiere
Copy link
Contributor Author

Thanks @scottmarlow , I rebased on your PR with your fixes, and also fixed CDI bean detection in Hibernate Search tests. Tests pass locally, let's see about CI...

@scottmarlow
Copy link
Contributor

@yrodiere I pushed more changes to #15165 including switching to ORM 6.0.0.Final.

@yrodiere
Copy link
Contributor Author

@scottmarlow Thanks for the heads-up, I rebased on your branch.

@scottmarlow
Copy link
Contributor

It looks like all of the test failures are for microprofile + clustering, I don't see any Hibernate test failures which is great!

@yrodiere
Copy link
Contributor Author

Ah, I didn't know someone else was watching the build results. Sorry about the back and forth, I'm struggling to find the right profiles where to disable tests (disable HSearch tests with the security manager, disable HSearch 5 tests when testing EE9, etc.).

It seems we're almost there, indeed. I'll look into the remaining failures... there's one related to HSearch with the security manager, in particular.

@yrodiere
Copy link
Contributor Author

Ok, this should remove the last remaining failure related to Hibernate Search (5, in this case).

Do I need to do something about the gazilion failures related to microprofile + clustering? Or is this something that already fails on your PR or the main branch?

@scottmarlow
Copy link
Contributor

Ok, this should remove the last remaining failure related to Hibernate Search (5, in this case).

Do I need to do something about the gazilion failures related to microprofile + clustering? Or is this something that already fails on your PR or the main branch?

No, you can ignore those failures as I don't believe they should be related to the Hibernate Search upgrade.

@yrodiere
Copy link
Contributor Author

Ok. It seems there are even fewer failures after rebasing on main; none are related to Hibernate Search.

This PR is ready as far as I'm concerned. Since your PR will have test failures until Hibernate Search gets upgraded, maybe you should integrate this PR into your own? So that we'll eventually merge the ORM 6 + Search 6 upgrades simultaneously.

@scottmarlow
Copy link
Contributor

Ok. It seems there are even fewer failures after rebasing on main; none are related to Hibernate Search.

This PR is ready as far as I'm concerned. Since your PR will have test failures until Hibernate Search gets upgraded, maybe you should integrate this PR into your own? So that we'll eventually merge the ORM 6 + Search 6 upgrades simultaneously.

@bstansberry @darranl do you have a preference for whether #15165 + #15185 are merged as one or separate pull requests?

#15165 does not fix one of the Hibernate Search test failures but that will be addressed by #15185.

@scottmarlow
Copy link
Contributor

@yrodiere 108f013#diff-74bfacd78cff4d8815bf930dda24f8402d5b0d2741ae39de85a355ff243da292 adds org.junit.Ignore to disable the testsuite/integration/basic/src/test/java/org/jboss/as/test/integration/hibernate/search/HibernateSearchJPATestCase.java Search test (see comment Ignore until we upgrade to Hibernate Search 6.1 via WFLY-15838).

After #15165 is merged and you sync up, could you please remove the @ignore from testsuite/integration/basic/src/test/java/org/jboss/as/test/integration/hibernate/search/HibernateSearchJPATestCase.java since your pull request will allow that test to work again.

Thanks

@yrodiere
Copy link
Contributor Author

yrodiere commented May 6, 2022

@scottmarlow Thanks for the heads-up. I rebased on your branch and removed the @Ignore annotation.

@yrodiere yrodiere marked this pull request as ready for review May 9, 2022 07:48
@yrodiere
Copy link
Contributor Author

yrodiere commented May 9, 2022

Hi @scottmarlow, I see your PR (#15165) was merged, so I rebased this one on main; it's ready for review. Please let me know if I need to change anything!

The only test failures before my rebase seemed unrelated (something about JMS and something about the WildFly client); I don't expect anything different after the rebase, since I didn't change anything. But let's see.

It's a dependency of Lucene 8.x and Hibernate Search 6.x.
…client

It's a dependency of Hibernate Search 6.x's Elasticsearch backend.
So that we can add more tests in sibling packages.
@yrodiere
Copy link
Contributor Author

Thanks @scottmarlow. I addressed all your comments (copyright years, JIRA ticket for TODOs). Please let me know if there's anything else.

@bstansberry
Copy link
Contributor

@yrodiere The Linux Security Manager CI job failures are due to https://issues.redhat.com/browse/WFCORE-5908. Why this PR has surfaced this bug needs investigation though. The failure indicates an earlier test (org.jboss.as.test.integration.ejb.mdb.vaultedproperties.MDBWithVaultedPropertiesTestCase) is not properly cleaning up after itself, but we only see that when testing this PR.

<execution>
<id>basic-integration-default-web.surefire</id>
<configuration>
<excludes>
Copy link
Contributor

Choose a reason for hiding this comment

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

I believe adding 'combine.children="append"' will fix the CI failure. This profile is superseding the entire list of excludes from this execution instead of just adding to it.

Copy link
Contributor

Choose a reason for hiding this comment

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

Never mind; that doesn't work.

There are only 3 tests involved here so it's better to drop the pom stuff and add @BeforeClass methods that call org.jboss.as.test.shared.util.AssumeTestGroupUtil.assumeSecurityManagerDisabled()

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There are only 3 tests involved here so it's better to drop the pom stuff and add @BeforeClass methods that call org.jboss.as.test.shared.util.AssumeTestGroupUtil.assumeSecurityManagerDisabled()

Done; thanks for the hint, I was looking for something like this :)

@yrodiere yrodiere requested a review from bstansberry May 11, 2022 11:09
@yrodiere
Copy link
Contributor Author

Thanks @bstansberry , I applied the changes you requested and the previously failing tests now pass. I think we're good to merge?

Copy link
Contributor

@bstansberry bstansberry left a comment

Choose a reason for hiding this comment

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

@yrodiere Looks good, but I want to confirm that the jboss-api settings in the various module.xml files are as expected. Most are not set, making the module public API.

(Note that even if things are public in WF, EAP can use jboss-api="unsupported" downstream if it doesn't want to support direct use of a module.)

@yrodiere
Copy link
Contributor Author

yrodiere commented May 12, 2022

@bstansberry I confirm jboss-api settings are as intended.

To clarify the intent:

  • The following modules are completely internal and not expected to appear in any public API, thus they are declared as private:
    • com.carrotsearch.hppc
    • org.apache.avro
  • The following modules can be useful to users of Hibernate Search, but direct use is not expected to be supported in EAP. We could declare them as public in WildFly and unsupported in EAP, if you prefer.
    • org.apache.lucene
    • org.elasticsearch.client.rest-client
    • com.google.code.gson
  • The following modules contain public Hibernate Search APIs that are absolutely necessary to Hibernate Search users, and thus they are public.
    • org.hibernate.search.engine
    • org.hibernate.search.mapper.orm
    • org.hibernate.search.mapper.pojo-base
  • The backend modules also contain public Hibernate Search APIs that are absolutely necessary to Hibernate Search users. However, they include APIs that involve types from private/unsupported modules (see above): those methods won't be usable unless users explicitly import unsupported/private modules (org.apache.lucene or org.elasticsearch.client.rest-client). This is as intended. See for example org.jboss.as.test.integration.hibernate.search.backend.lucene.extension.HibernateSearchLuceneDependencyTestCase or org.jboss.as.test.integration.hibernate.search.backend.elasticsearch.extension.HibernateSearchElasticsearchGsonDependencyTestCase.
    • org.hibernate.search.backend.elasticsearch
    • org.hibernate.search.backend.lucene
  • Some artifacts don't even have a JBoss module because they are considered incubating or should not be used in production environments. We could add modules declared as public or private in WildFly and unsupported in EAP, if you prefer. See for example org.jboss.as.test.integration.hibernate.search.v5migrationhelper.simple.HibernateSearchV5MigrationHelperTestCase or org.jboss.as.test.integration.hibernate.search.coordination.HibernateSearchOutboxPollingTestCase.
    • hibernate-search-mapper-orm-coordination-outbox-polling
    • hibernate-search-v5migrationhelper-engine
    • hibernate-search-v5migrationhelper-orm

Copy link
Contributor

@bstansberry bstansberry left a comment

Choose a reason for hiding this comment

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

@yrodiere Thanks so much for such a thorough answer! If you don't mind I'll copy that into a comment in WFLY-15838 as it's excellent design documentation and will remain more visible there.

@bstansberry bstansberry merged commit de954e6 into wildfly:main May 12, 2022
@yrodiere
Copy link
Contributor Author

No problem, thanks for merging :)

@yrodiere yrodiere changed the title WFLY-15838 + WFLY-15839 + WFLY-15840 + WFLY-15841 + WFLY-15842 Upgade to Hibernate Search 6 (EE 9 + ORM 6) WFLY-15838 + WFLY-15839 + WFLY-15840 + WFLY-15841 + WFLY-15842 Upgrade to Hibernate Search 6 (EE 9 + ORM 6) Jan 20, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
deps-changed Dependencies have been checked, and there are changes highlighted in a comment
Projects
None yet
3 participants