SqlClient pool migration

[tsegismont]

PgPool and other vendor pool types are deprecated in Vert.x 4. In Vert.x 5, these types are going away.

This PR is an attempt to make the transition incremental. Instead of using PgPool and other vendor pool types as the basis for bean creation, we can use the Pool parent type.

In the recorder, the pool configurator now returns the parent Pool type.
Then, in the processor, new bean configurators are set up to support injection of the different pools:

• bare pool
• vendor pool
• mutiny pool
• mutiny vendor pool

Each commit can be reviewed individually.

Updated extensions:

• Reactive Pg Client
• Reactive MySQL Client
• Reactive SqlServer Client
• Reactive Oracle Client
• Reactive DB2 Client

[tsegismont]

@cescoffier @gsmet @yrodiere @geoand @jponge would you mind taking a look at this draft PR?

There are a few things I would like to figure out before going on with other extensions.

DRY

This is maybe a dumb question.

In the processor there is now:

    private static final DotName VERTX_POOL = DotName.createSimple(Pool.class);
    private static final Type VERTX_POOL_TYPE = Type.create(VERTX_POOL, Type.Kind.CLASS);

Is it fine to put this somewhere in the reactive-datasource extension to avoid duplication?

Injection of vendor pool types

Should we have a warning message logged to the console when this happens?
In this case, how about adding the log instruction inside the body of the function defined in Recorder#vendorPool and Recorder#mutinyVendorPool?

Implications on Hibernate Reactive

Since both the raw and vendor types can still be injected, I don't believe Hibernate Reactive integration will be impacted.

Migration tooling

Do we share OpenRewrite rules somewhere? How about adding rules so that users can start migrating now?

Datasource Health Check filtering

ReactivePgDataSourcesHealthCheck still looks up for PgPool in Arc. I couldn't think about a solution for selecting pools bound to datasources having the correct db kind. Any ideas?

Mutiny types are not deprecated

Both io.vertx.mutiny.sqlclient.Pool and io.vertx.mutiny.pgclient.PgPool aren't deprecated, even though the corresponding Vert.x type are deprecated.

Shouldn't we fix this in Vert.x Mutiny bindings?

I would expect this to urge users to switch to parent pool types in their codebases.

[geoand]

Since both the raw and vendor types can still be injected, I don't believe Hibernate Reactive integration will be impacted.

cc @DavideD

Do we share OpenRewrite rules somewhere? How about adding rules so that users can start migrating now?

cc @gsmet

Is it fine to put this somewhere in the reactive-datasource extension to avoid duplication?

Sure, you can a ReactiveDataSourceDotNames class

Shouldn't we fix this in Vert.x Mutiny bindings?

cc @jponge

[github-actions]

🙈 The PR is closed and the preview is expired.

[jponge]

@tsegismont re Mutiny types are not deprecated yes it's something to be considered

[jponge]

Note that the Likely new bindings generator does honor deprecation annonations.

[yrodiere]

Hey,

Thanks, I added a few comments below.

In the processor there is now:

    private static final DotName VERTX_POOL = DotName.createSimple(Pool.class);
    private static final Type VERTX_POOL_TYPE = Type.create(VERTX_POOL, Type.Kind.CLASS);

Is it fine to put this somewhere in the reactive-datasource extension to avoid duplication?

Not only is it fine, it's great! Every time I touch this code I think "man, so much duplication..."

Injection of vendor pool types

Should we have a warning message logged to the console when this happens? In this case, how about adding the log instruction inside the body of the function defined in Recorder#vendorPool and Recorder#mutinyVendorPool?

I think it's probably enough if the types themselves are deprecated?

But if you want warnings at Quarkus build time or at bootstrap, maybe we should ask @Ladicek / @mkouba / @manovotn. What is the recommended way to mark a bean as "dangerous" and/or have warnings be logged when it's injected/retrieved?

Implications on Hibernate Reactive

Since both the raw and vendor types can still be injected, I don't believe Hibernate Reactive integration will be impacted.

I think Hibernate Reactive uses Pool already:

quarkus/extensions/hibernate-reactive/runtime/src/main/java/io/quarkus/hibernate/reactive/runtime/FastBootHibernateReactivePersistenceProvider.java

Lines 275 to 295 in 3e63622

	private void registerVertxAndPool(String persistenceUnitName,
	RuntimeSettings runtimeSettings,
	PreconfiguredReactiveServiceRegistryBuilder serviceRegistry) {
	if (runtimeSettings.isConfigured(AvailableSettings.URL)) {
	// the pool has been defined in the persistence unit, we can bail out
	return;
	}
	// for now, we only support one pool but this will change
	String datasourceName = DataSourceUtil.DEFAULT_DATASOURCE_NAME;
	Pool pool;
	try {
	InjectableInstance<Pool> poolHandle = Arc.container().select(Pool.class);
	if (!poolHandle.isResolvable()) {
	throw new IllegalStateException("No pool has been defined for persistence unit " + persistenceUnitName);
	}
	// ClientProxy.unwrap is necessary to trigger exceptions on inactive datasources
	pool = ClientProxy.unwrap(poolHandle.get());
	} catch (RuntimeException e) {
	throw PersistenceUnitUtil.unableToFindDataSource(persistenceUnitName, datasourceName, e);
	}

Migration tooling

Do we share OpenRewrite rules somewhere? How about adding rules so that users can start migrating now?

https://github.com/quarkusio/quarkus-updates/

Datasource Health Check filtering

ReactivePgDataSourcesHealthCheck still looks up for PgPool in Arc. I couldn't think about a solution for selecting pools bound to datasources having the correct db kind. Any ideas?

I'd expect something in the (build-time) processor to take note of which datasources are PostgreSQL datasources, create a List<String> with the names, and have it passed to ReactivePgDataSourcesHealthCheck somehow, so it'll retrieve those datasources and those only.

Mutiny types are not deprecated

Both io.vertx.mutiny.sqlclient.Pool and io.vertx.mutiny.pgclient.PgPool aren't deprecated, even though the corresponding Vert.x type are deprecated.

Shouldn't we fix this in Vert.x Mutiny bindings?

I would expect this to urge users to switch to parent pool types in their codebases.

+1

[Ladicek]

 private static final Type VERTX_POOL_TYPE = Type.create(VERTX_POOL, Type.Kind.CLASS);

Just for the record, you can simplify that to ClassType.create(VERTX_POOL).

But if you want warnings at Quarkus build time or at bootstrap, maybe we should ask @Ladicek / @mkouba / @manovotn. What is the recommended way to mark a bean as "dangerous" and/or have warnings be logged when it's injected/retrieved?

I don't think we have anything like that. How would we detect that? Application classes would trigger the warning, but non-application classes wouldn't? Would we detect that on beans or on bean types? (As in, some bean types would be OK to use, while others would be "dangerous".)

[DavideD]

Since both the raw and vendor types can still be injected, I don't believe Hibernate Reactive integration will be impacted.

cc @DavideD

Hibernate Reactive doesn't and shouldn't use PgPool, because we support other databases.

[manovotn]

 private static final Type VERTX_POOL_TYPE = Type.create(VERTX_POOL, Type.Kind.CLASS);

Just for the record, you can simplify that to ClassType.create(VERTX_POOL).

But if you want warnings at Quarkus build time or at bootstrap, maybe we should ask @Ladicek / @mkouba / @manovotn. What is the recommended way to mark a bean as "dangerous" and/or have warnings be logged when it's injected/retrieved?

I don't think we have anything like that. How would we detect that? Application classes would trigger the warning, but non-application classes wouldn't? Would we detect that on beans or on bean types? (As in, some bean types would be OK to use, while others would be "dangerous".)

Ladislav is right, there is no facility for that.
But if you are the bean "owner", you can always add some logging to calls such as @PostConstruct which would be invoked any time that bean is created. If it's a synth. bean, you can add it to its create callback with the same result.

Only other thing I can think of is monitoring injection points in user app and then logging; I think we had some build item with all injection points? This of course won't detect/include any dynamic resolution (Instance<T>, Arc.container().select(), CDI.current()).

[jponge]

FYI I'm doing a patch release soon of the Vert.x Mutiny bindings, and the fix for deprecation will be in (+ using the relocated Jandex plugin that for some reason we hadn't migrated to)

[tsegismont]

Thanks everyone for your comments!

Not only is it fine, it's great! Every time I touch this code I think "man, so much duplication..."

@yrodiere I have the same feeling. Perhaps it's better to address this in a separate PR though.

I'd expect something in the (build-time) processor to take note of which datasources are PostgreSQL datasources, create a List<String> with the names, and have it passed to ReactivePgDataSourcesHealthCheck somehow, so it'll retrieve those datasources and those only.

@yrodiere thanks, I'll try that.

Just for the record, you can simplify that to ClassType.create(VERTX_POOL).

@Ladicek thanks for the tip

Ladislav is right, there is no facility for that.
But if you are the bean "owner", you can always add some logging to calls such as @PostConstruct which would be invoked any time that bean is created. If it's a synth. bean, you can add it to its create callback with the same result.

@manovotn @Ladicek thanks. Since the Mutiny Vert.x bindings generator will soon be able to mark generated types as deprecated when needed, I won't pursue in this direction.

FYI I'm doing a patch release soon of the Vert.x Mutiny bindings, and the fix for deprecation will be in (+ using the relocated Jandex plugin that for some reason we hadn't migrated to)

Thank you @jponge !

[tsegismont]

@yrodiere PTAL, in particular to the commit that get rids of vendor pool lookup in ReactivePgDataSourcesHealthCheck.

[mkouba]

Only other thing I can think of is monitoring injection points in user app and then logging; I think we had some build item with all injection points? This of course won't detect/include any dynamic resolution (Instance<T>, Arc.container().select(), CDI.current()).

Yes, there's the BeanDiscoveryFinishedBuildItem, and the BeanDiscoveryFinishedBuildItem#getInjectionPoints() method returns all injection points declared on class beans. There's also SynthesisFinishedBuildItem which also includes synthetic injection points.

[tsegismont]

@gsmet @yrodiere @geoand can you please take a look?

I thought about addressing code duplication in SQL Client extensions in a follow-up PR, is that fine with you?

[tsegismont]

@geoand ^^

[geoand]

@tsegismont that's exactly what I had in mind!

[tsegismont]

@tsegismont that's exactly what I had in mind!

@geoand done, and squashed commits

[geoand]

Thanks! I'll have another look tomorrow

[quarkus-bot]

Status for workflow Quarkus Documentation CI

This is the status report for running Quarkus Documentation CI on commit a07d65d.

✅ 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.

Warning

There are other workflow runs running, you probably need to wait for their status before merging.

[quarkus-bot]

Status for workflow Quarkus CI

This is the status report for running Quarkus CI on commit a07d65d.

✅ 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.

[geoand]

Thanks!

[xstefank]

LGTM

[tsegismont]

Thank you guys. Can one of you please merge the PR?

[geoand]

I'll want to give @gsmet a chance to review if he wants to

[gsmet]

Sorry, late to the party, will have a look this afternoon and hopefully merge it.

[gsmet]

I merged it for 3.21.

A few additional things to do:

• Properly document the deprecations with since = "3.21"
• Post something for the migration guide (in Asciidoc), you can post it here
• I think it might be a good idea to add some recipes to https://github.com/quarkusio/quarkus-updates/tree/main/recipes/src/main/resources/quarkus-updates/core . We just need to adjust the types, the API is exactly the same? If so, it should be easy.

[tsegismont]

Thank you @gsmet

Here's an update for the migration guide:

== Datasources

=== Generic pool class for reactive datasources

In previous versions of Quarkus, there were vendor-specific pool classes for each reactive datasource type:

* `io.vertx.mutiny.db2client.DB2Pool`, for IBM Db2
* `io.vertx.mutiny.mysqlclient.MySQLPool`, for MariaDB/MySQL
* `io.vertx.mutiny.mssqlclient.MSSQLPool`, for Microsoft SQL Server
* `io.vertx.mutiny.oracleclient.OraclePool`, for Oracle
* `io.vertx.mutiny.pgclient.PgPool`, for PostgreSQL.

Starting with this version of Quarkus, you should use the generic pool class, `io.vertx.mutiny.sqlclient.Pool`, regardless of the database vendor.

Regarding recipes, changing imports should be fine for all "regular" users, i.e. users injecting a managed reactive pool.
Advanced users (e.g. creating pools manually) might see their code broken in some cases. The impact should be limited though, because you usually don't create pools in each and every class.
Is that acceptable?