Add Hibernate Search management endpoint

[marko-bekhta]

Fixes #34553

Opening this as a draft so we can discuss it. A couple of questions from my side:

• Is it ok for management endpoints to be more complex than simple GET, if so
  • Do we want the mass-indexer configuration to be available at all? For now, I've added it as config properties, but I was wondering if we maybe should move it to the body of a POST request or maybe allow both options?
  • Do we want to be more flexible and allow indexing not all but a subset of entities? There's a filter in a query params for now that can do it, but again, maybe it would be better to move it to the request body?
  • Same about tenants, with an improvement we've added to Search mass-indexer should pick all tenants automatically, but maybe we want to have a filter for tenants too?
• How about more granular operations like reindexing a specific entity (by ID)? We could do it through an indexing plan...

I remember implementing similar logic in the webapps quite often, and if these endpoints would be available to an admin in prod mode, it could save people some time 😃

[quarkus-bot]

/cc @gsmet (hibernate-search), @yrodiere (hibernate-search)

[yrodiere]

Thanks! Comments below.

Is it ok for management endpoints to be more complex than simple GET

Well for a start, in this case it most definitely should be a POST, because GET semantics are not a good match :)

And I guess you should be able to accept a payload, yes.

Do we want the mass-indexer configuration to be available at all

It's certainly useful, I'm just not sure we want to maintain this.

But if you've already started working on this... let's say "yes, but be sure to mark this as preview technology in the documentation"? I don't see any documentation, btw :)

Do we want to be more flexible and allow indexing not all but a subset of entities? There's a filter in a query params for now that can do it, but again, maybe it would be better to move it to the request body?

Makes sense. I'd suggest relying on entity type names: searchMapping.scope( Object.class, Arrays.asList( "entityName1", "entityName2" ) )

Same about tenants, with an improvement we've added to Search mass-indexer should pick all tenants automatically, but maybe we want to have a filter for tenants too?

Makes sense.

How about more granular operations like reindexing a specific entity (by ID)? We could do it through an indexing plan...

Looks interesting, and indeed I've used such things myself, but I'd suggest creating a separate ticket as we need to refine the requirements: should this trigger reindexing of containing classes (in which case IndexingPlan makes sense)? Or just the entity we're being pointed to (in which case MassIndexer with a condition is more appropriate).

I remember implementing similar logic in the webapps quite often

I'd be interested in discussing those webapps (offline); I thought you hadn't used Hibernate Search much before joining the team :)

[marko-bekhta]

Hey Yoann 😃
Here's the next iteration on this one.

• I've added support for non-default PUs through an optional query param.
• There's some documentation now.
• Moved the config of the mass indexer to the body of the request.
• Same for filters on tenants or types to reindex -- they are now in the request body too.

I've also noticed that there seems to be a problem with Flyway... quarkus.flyway.clean-at-start cleans the records (including the ones in the flyway_schema_history, but it keeps the tables. This means if the tests are executed against the same container, they try to run the migration, but that fails because all the DB objects are still there... so I've added a couple of drop statements to the script to address the issue for the tests. But overall this looks like a bug to me...

[github-actions]

🙈 The PR is closed and the preview is expired.

[yrodiere]

Thanks. A few comments below.

I've also noticed that there seems to be a problem with Flyway... quarkus.flyway.clean-at-start cleans the records (including the ones in the flyway_schema_history, but it keeps the tables. This means if the tests are executed against the same container, they try to run the migration, but that fails because all the DB objects are still there... so I've added a couple of drop statements to the script to address the issue for the tests. But overall this looks like a bug to me...

No idea what is going on. Maybe open a bug report?

[yrodiere]

Thanks @marko-bekhta , LGTM. I'll mark this as ready for review, since... it's already been reviewed :) Let's see what CI has to say about it.

[yrodiere]

@marko-bekhta Tests seem to fail, see ^

[marko-bekhta]

yeah 😔

java.lang.NoClassDefFoundError: io/quarkus/vertx/http/deployment/RouteBuildItem
	at java.base/java.lang.Class.forName0(Native Method)
	at java.base/java.lang.Class.forName(Class.java:496)

this is coming from the

        <dependency>
            <groupId>io.quarkus</groupId>
            <artifactId>quarkus-vertx-http-deployment</artifactId>
            <optional>true</optional>
        </dependency>

being an optional dependency... But I'm not sure whether it can be non-optional in quarkus-hibernate-search-orm-elasticsearch-deployment. Should a management part be an additional artifact?

The other failure is from the label change... so that's easier to fix 😃

[yrodiere]

java.lang.NoClassDefFoundError: io/quarkus/vertx/http/deployment/RouteBuildItem
	at java.base/java.lang.Class.forName0(Native Method)
	at java.base/java.lang.Class.forName(Class.java:496)

this is coming from the

        <dependency>
            <groupId>io.quarkus</groupId>
            <artifactId>quarkus-vertx-http-deployment</artifactId>
            <optional>true</optional>
        </dependency>

being an optional dependency...

Ok, makes sense.

But I'm not sure whether it can be non-optional in quarkus-hibernate-search-orm-elasticsearch-deployment.

It is non-optional for Micrometer at least:

quarkus/extensions/micrometer/deployment/pom.xml

Lines 35 to 38 in 351195a

	<dependency>
	<groupId>io.quarkus</groupId>
	<artifactId>quarkus-vertx-http-deployment</artifactId>
	</dependency>

If we're talking about the deployment artifact only, I think the non-optional dependency is fine? It won't be present at runtime, only at build time.

TBH I'm not sure why the dependency was optional in the first place.

Should a management part be an additional artifact?

No I don't think we need an additional artifact here.

[marko-bekhta]

Hey hey @cescoffier

We are trying to add some optional management endpoints to the Hibernate Search Extension. Having

<dependency>
    <groupId>io.quarkus</groupId>
    <artifactId>quarkus-vertx-http-deployment</artifactId>
    <optional>true</optional>
</dependency>

as a dependency in build (and quarkus-vertx-http in runtime) module of the extension works for as long as quarkus-vertx-http is also available at runtime. Otherwise the build fails with

java.lang.NoClassDefFoundError: io/quarkus/vertx/http/deployment/RouteBuildItem
	at java.base/java.lang.Class.forName0(Native Method)
	at java.base/java.lang.Class.forName(Class.java:496)

Making the build dependency required while leaving the runtime dependency as optional doesn't work since it leads to:

[ERROR] Failed to execute goal io.quarkus:quarkus-extension-maven-plugin:999-SNAPSHOT:extension-descriptor (generate-extension-descriptor) on project quarkus-hibernate-search-orm-elasticsearch: The deployment artifact io.quarkus:quarkus-hibernate-search-orm-elasticsearch-deployment::jar depends on the following Quarkus extension deployment artifacts whose corresponding runtime artifacts were not found among the dependencies of io.quarkus:quarkus-hibernate-search-orm-elasticsearch:jar:999-SNAPSHOT: io.quarkus:quarkus-netty-deployment::jar io.quarkus:quarkus-vertx-deployment::jar io.quarkus:quarkus-vertx-http-deployment::jar io.quarkus:quarkus-virtual-threads-deployment::jar

Yoann suggested to ping you for an idea/example on how we could make these management endpoints optional and only available if the user has quarkus-vertx-http without making it a mandatory dependency of the Hibernate Search extension. Any advice or pointers would be much appreciated. Thanks!

[yrodiere]

To clarify, I suspect that we would need to move RouteBuildItem to an SPI module (or add a similar build item to an SPI module) so that we can have a dependency in our deployment module only. But I wondered if you already faced this problem and already have a solution @cescoffier .

[cescoffier]

I need to think about it. We can move it to an SPI module, but there is more than this build item. How it's managed (main router, application router, management endpoint) is particularly tricky, and I think it will drag a bit of this logic in the SPI (which is not something great, as it's an internal detail).

Don't we have other cases where the routes are optional? In micrometer, we use a lot of optional extensions (so, I would look into a pattern there).

[yrodiere]

Don't we have other cases where the routes are optional? In micrometer, we use a lot of optional extensions (so, I would look into a pattern there).

From what I can see Micrometer depends on "optional" extensions at runtime only (no build items required from those optional extensions). In our case we need to depend on quarkus-vertx-http at build time, too...

@marko-bekhta did you try a non-optional dependency at build time (deployment module) + an optional dependency at runtime (runtime module)?

[marko-bekhta]

did you try a non-optional dependency at build time (deployment module) + an optional dependency at runtime (runtime module)?

Yeah 😔, that's when that maven build failure happens because of the check:

[INFO] 12 goals, 12 executed
[ERROR] Failed to execute goal io.quarkus:quarkus-extension-maven-plugin:999-SNAPSHOT:extension-descriptor (generate-extension-descriptor) on project quarkus-hibernate-search-orm-elasticsearch: The deployment artifact io.quarkus:quarkus-hibernate-search-orm-elasticsearch-deployment::jar depends on the following Quarkus extension deployment artifacts whose corresponding runtime artifacts were not found among the dependencies of io.quarkus:quarkus-hibernate-search-orm-elasticsearch:jar:999-SNAPSHOT: io.quarkus:quarkus-netty-deployment::jar io.quarkus:quarkus-vertx-deployment::jar io.quarkus:quarkus-vertx-http-deployment::jar io.quarkus:quarkus-virtual-threads-deployment::jar -> 

to be clear, that's when I'd put in quarkus-hibernate-search-orm-elasticsearch-deployment

        <dependency>
            <groupId>io.quarkus</groupId>
            <artifactId>quarkus-vertx-http-deployment</artifactId>
        </dependency>

and quarkus-hibernate-search-orm-elasticsearch will have:

        <dependency>
            <groupId>io.quarkus</groupId>
            <artifactId>quarkus-vertx-http</artifactId>
            <optional>true</optional>
        </dependency>

[yrodiere]

Right. I guess that makes sense since quarkus-vertx-http-deployment could trigger generation of bytecode that depends on its runtime, so we can't allow the runtime to be optional.

Hence the suggestion to introduce an SPI... if that SPI only declares dependencies + build items, and does not trigger any bytecode generation by itself, then it becomes fine to depend on that SPI in -deployment, and on the quarkus-vertx-http runtime in our own runtime (with <optional>true</optional>).

[yrodiere]

Hey @cescoffier, any news on that quarkus-vertx-http-deployment-spi that would enable declaring management interfaces without pulling in vertx-http into all extensions?
Should we create an issue about it?

[cescoffier]

Please create an issue, I didn't look at it yet.

[yrodiere]

Please create an issue, I didn't look at it yet.

Done: #37144

[yrodiere]

Yay, it works :)

Let's wait for #38143 to be merged first though :)

[gsmet]

I rebased it on top of Clément's PR, now that it has been merged.

[marko-bekhta]

Thanks! I was about to go fetch a laptop to rebase it 🙈 😃

[quarkus-bot]

✔️ The latest workflow run for the pull request has completed successfully.

It should be safe to merge provided you have a look at the other checks in the summary.

You can consult the Develocity build scans.

[yrodiere]

Merging, thanks everyone!