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

Unactionable warning when booting Hibernate Reactive #45263

Open
gsmet opened this issue Dec 23, 2024 · 7 comments
Open

Unactionable warning when booting Hibernate Reactive #45263

gsmet opened this issue Dec 23, 2024 · 7 comments
Labels
area/hibernate-orm Hibernate ORM area/hibernate-reactive Hibernate Reactive kind/question Further information is requested

Comments

@gsmet
Copy link
Member

gsmet commented Dec 23, 2024

When running the Hibernate Reactive ITs in integration-tests/hibernate-reactive-postgresql, I get the following warning:

2024-12-23 20:12:48,962 WARN [io.qua.hib.orm.run.ser.QuarkusRuntimeInitDialectFactory] (JPA Startup Thread) Persistence unit default-reactive: Could not retrieve the database version to check it is at least 12.0.0

Two questions:

  • I'm not sure this is normal as I don't have the same warning with Hibernate ORM in integration-tests/jpa-h2.
  • Should we improve the warning to make it more actionable? I think in this case, we should probably set the version ourselves in the configuration?
@gsmet gsmet added area/hibernate-orm Hibernate ORM area/hibernate-reactive Hibernate Reactive kind/question Further information is requested labels Dec 23, 2024
Copy link

quarkus-bot bot commented Dec 23, 2024

/cc @DavideD (hibernate-reactive), @gavinking (hibernate-reactive)

@yrodiere
Copy link
Member

yrodiere commented Jan 2, 2025

I'm not sure this is normal as I don't have the same warning with Hibernate ORM in integration-tests/jpa-h2.

This typically happens when we're not able to reach the DB on startup, and I don't think we have such test cases for ORM -- it is technically not supported yet.

I think in this case, we should probably set the version ourselves in the configuration?

I don't think so, since the version we're not able to retrieve is the actual version of the DB we're connecting to. No way to hardcode that.

Should we improve the warning to make it more actionable?

We could add a hint such as "is the database reachable?", but beyond that there's not much we can do. This warning happens either if the DB is unreachable on startup (which is technically not supported yet) or if there is a bug on our side.

In this case I wonder if Hibernate Reactive just doesn't have the same version detection code that ORM has, at least when the datasource isn't JDBC-enabled. Because the version detection code in ORM is purely JDBC-based.
Or we (I) simply didn't wire the detection properly for Hibernate Reactive in Quarkus.
Maybe @DavideD can have a look?

FWIW the detection happens here in Quarkus:

private Optional<DatabaseVersion> retrieveDbVersion(DialectResolutionInfoSource resolutionInfoSource) {
var databaseMetadata = resolutionInfoSource == null ? null
: resolutionInfoSource.getDialectResolutionInfo().getDatabaseMetadata();
if (databaseMetadata == null) {
return Optional.empty();
}
try {
triedToRetrieveDbVersion = true;
return Optional.of(DatabaseVersion.make(databaseMetadata.getDatabaseMajorVersion(),
databaseMetadata.getDatabaseMinorVersion()));
} catch (RuntimeException | SQLException e) {
LOG.warnf(e, "Persistence unit %1$s: Could not retrieve the database version to check it is at least %2$s",
persistenceUnitName, buildTimeDbVersion);
return Optional.empty();
}
}
}

And is fed information generated by this code in Hibernate ORM: https://github.com/hibernate/hibernate-orm/blob/748fa8f7e1cafae4d60618d9082e3523ae3cf7b2/hibernate-core/src/main/java/org/hibernate/engine/jdbc/env/internal/JdbcEnvironmentInitiator.java#L162-L171

Note that we'll change that code a bit once we upgrade to ORM7, so that the dialect itself is in charge of detecting the version (which it can do more finely): https://github.com/quarkusio/quarkus/pull/43764/files#diff-86d161403825b101dd6e16f36ea3588690195d15079b74becd72409f3ab173a5L109-R109

@DavideD
Copy link
Contributor

DavideD commented Jan 8, 2025

In this case I wonder if Hibernate Reactive just doesn't have the same version detection code that ORM has

Yes, Hibernate Reactive has it's own way to detect the database: https://github.com/hibernate/hibernate-reactive/blob/37d57f6429fc20c5cc1e30f5fc22c6cde343d714/hibernate-reactive-core/src/main/java/org/hibernate/reactive/provider/service/NoJdbcEnvironmentInitiator.java#L81

We basically recreate the dialect but override the resolution info.

Quarkus seems to skip some steps when using Hibernate Reactive (it makes sense because we don't use JDBC) and QuarkusRuntimeInitDialectFactory#retrieveDbVersion is never called.

Note that we'll change that code a bit once we upgrade to ORM7, so that the dialect itself is in charge of detecting the version

Let's try not to forget about Hibernate Reactive when we do this.

@yrodiere
Copy link
Member

yrodiere commented Jan 8, 2025

Note that we'll change that code a bit once we upgrade to ORM7, so that the dialect itself is in charge of detecting the version

Let's try not to forget about Hibernate Reactive when we do this.

Can you please add a rejecting review on my PR mentioning that, so I don't forget?

#45263

@gsmet
Copy link
Member Author

gsmet commented Jan 8, 2025

Quarkus seems to skip some steps when using Hibernate Reactive (it makes sense because we don't use JDBC) and QuarkusRuntimeInitDialectFactory#retrieveDbVersion is never called.

But doesn't the warning come from this very code?

FWIW, I'm running the tests with a container started so I would expect them to be able to contact the database.

@DavideD
Copy link
Contributor

DavideD commented Jan 8, 2025

No, the warning comes from QuarkusRuntimeInitDialectFactory#checkActualDbVersion, and the log happens because QuarkusRuntimeInitDialectFactory#buildDialect has not being called before (and therefore the actual db version has not been initialized).

@DavideD
Copy link
Contributor

DavideD commented Jan 8, 2025

Can you please add a rejecting review on my PR mentioning that, so I don't forget?

#45263

Did you mean here? #43764

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/hibernate-orm Hibernate ORM area/hibernate-reactive Hibernate Reactive kind/question Further information is requested
Projects
None yet
Development

No branches or pull requests

3 participants