-
Notifications
You must be signed in to change notification settings - Fork 2.7k
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
Use Arc features in datasource extensions for eager startup and active/inactive #41929
Use Arc features in datasource extensions for eager startup and active/inactive #41929
Conversation
Thanks for your pull request! Your pull request does not follow our editorial rules. Could you have a look?
This message is automatically generated by a bot. |
/cc @gsmet (hibernate-orm) |
Actually I'll keep this as a draft... the base commit makes changes in Arc, which probably means the rebuild will be massive, even if I only need to test things related to datasources :/ |
This comment has been minimized.
This comment has been minimized.
8fe1856
to
5ffad3c
Compare
705e007
to
645c876
Compare
41fe36b
to
efac1c7
Compare
Hey @Ladicek, FYI I made some progress here regarding integration with your work in #41810, but didn't find the time to finish before my PTO. Sorry... I'll resume my work when I'm back at the end of August. I'd personally not bother merging #41810 until then, but feel free if you're in a rush and are okay with what you're seeing here so far. I guess we can always iterate later on. See you! |
So that we correctly detect empty URLs.
Because it duplicates: * ConfigUrlMissingDefaultDatasourceStaticInjectionTest * ConfigUrlMissingDefaultDatasourceDynamicInjectionTest * ConfigUrlMissingNamedDatasourceStaticInjectionTest * ConfigUrlMissingNamedDatasourceDynamicInjectionTest
Consequently: * Skip the health check for datasources without a URL * Fail on startup if datasources without URL are (statically) injected into user bean * Fail on first access if datasources without URL are retrieved dynamically through CDI
…jected into user beans By reimplementing Flyway's/Liquibase's inactive/active handling and eager startup through Arc's native features, which is better integrated and gives us this behavior.
For consistency with datasource documentation.
00a940d
to
e5e300d
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Mention the "getActive()" solution in particular.
e5e300d
to
093b837
Compare
Status for workflow
|
Status for workflow
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rubber-stamped!
Merged! Thanks a lot to everyone involved :) |
Also, I updated the 3.17 migration guide: https://github.com/quarkusio/quarkus/wiki/Migration-Guide-3.17#datasources |
👍🏽 |
private final RuntimeValue<DataSourcesRuntimeConfig> runtimeConfig; | ||
private final RuntimeValue<DataSourcesReactiveRuntimeConfig> reactiveRuntimeConfig; | ||
|
||
@Inject |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
interesting, does jakarta.inject.Inject;
has any effect on records at all? I didn't know.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It doesn't really. If the annotation wasn't present, it would work exactly the same.
If anything, this is more of a documentation that injection happens. But it's way out of any specification, so perhaps it shouldn't be here after all 🤷
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
copy that, thanks
Based on #41810 , which must be merged first.=> DoneBest reviewed commit by commit.
Effects of this PR:
*DatasourceHealthCheckTest.java
for Agroal.quarkus.datasource.reactive.url
#43517: see changes in expected behavior in*DatasourceHealthCheckTest.java
and*ConfigUrlMissing*Test.java
for reactive SQL client extensions.Content to be added to the migration guide:
== Datasources [[datasource-no-default-url]] === No default URL for Reactive SQL clients In previous versions of Quarkus, when dev services were disabled, or when running in production mode, `quarkus.datasource.reactive.url`/`quarkus.datasource."datasource-name".reactive.url` would be implicitly set to an undocumented default targeting `localhost` and a DB-specific port. Starting with this version of Quarkus, when dev services are disabled, or when running in production mode, `quarkus.datasource.reactive.url`/`quarkus.datasource."datasource-name".reactive.url` no longer has a default: if the property is not set, the corresponding datasource will be deactivated, leading to the behavior described in <<datasource-inactive-fail-fast>>. If your application needs to target localhost for one of its datasources, set `quarkus.datasource.reactive.url`/`quarkus.datasource."datasource-name".reactive.url` explicitly, for example in `application.properties`. [[datasource-no-health-check-if-no-url]] === Datasources without a URL no longer contribute a health check In previous versions of Quarkus, when dev services were disabled, or when running in production mode, and when `quarkus.datasource.reactive.url`/`quarkus.datasource."datasource-name".reactive.url` was not set, a health check would be contributed for the corresponding datasource regardless, resulting in unreliable health checks that would either always succeed (JDBC datasource) or (almost) always fail (reactive datasources). Starting with this version of Quarkus, datasources without a URL will no longer contribute a health check. If your application needs a health check for a datasource, make sure to set `quarkus.datasource.jdbc.url`/`quarkus.datasource."datasource-name".jdbc.url`/`quarkus.datasource.reactive.url`/`quarkus.datasource."datasource-name".reactive.url` explicitly. NOTE: Quarkus will still attempt to detect misconfiguration on application startup, but will report it differently, potentially earlier: see <<datasource-inactive-fail-fast>>. [[datasource-inactive-fail-fast]] === Datasource usage fails fast if datasource is deactivated or has no URL set In previous versions of Quarkus, when dev services were disabled, or when running in production mode, and when the datasource was deactivated (`quarkus.datasource.active=false`) or had no URL set, the application would be allowed to start successfully, but could fail much later upon first access to the datasource. Starting with this version of Quarkus, datasources that are deactivated or do not have a URL will lead to a startup failure if Quarkus can detect they are used: * Static CDI injection points involving the datasource (`@Inject DataSource ds` or `@Inject Pool pool`) will cause application startup to fail with an explicit, actionable message. * Dynamic retrieval of the datasource (e.g. through `CDI.getBeanContainer()`/`Arc.instance()`, or by injecting an `Instance<DataSource>`) will not be detected on startup, but will cause an exception to be thrown with an explicit, actionable message. The <<flyway-inactive-fail-fast,Flyway>> and <<liquibase-inactive-fail-fast,Liquibase>> extensions implement a similar behavior for their `Flyway`/`LiquibaseFactory` CDI beans. If your application needs to inject a `DataSource`/`Flyway`/`LiquibaseFactory` bean for a datasource that can potentially be deactivated or have no URL, you may encounter a startup failure similar to this: ``` io.quarkus.arc.InactiveBeanException: Bean is not active: SYNTHETIC bean [class=io.agroal.api.AgroalDataSource, id=sqqLi56D50iCdXmOjyjPSAxbLu0] Reason: Datasource '<default>' was deactivated automatically because its URL is not set. To activate the datasource, set configuration property 'quarkus.datasource.jdbc.url'. Refer to https://quarkus.io/guides/datasource for guidance. To avoid this exception while keeping the bean inactive: - Configure all extensions consuming this bean as inactive as well, if they allow it, e.g. 'quarkus.someextension.active=false' - Make sure that custom code only accesses this bean if it is active - Inject the bean with 'Instance<io.agroal.api.AgroalDataSource>' instead of 'io.agroal.api.AgroalDataSource' This bean is injected into: -io.quarkus.agroal.test.ConfigUrlMissingDefaultDatasourceStaticInjectionTest$MyBean#ds at io.agroal.api.AgroalDataSource_sqqLi56D50iCdXmOjyjPSAxbLu0_Synthetic_Bean.doCreate(Unknown Source) [...] ``` To avoid this failure, follow the instructions in the exception message. In particular, consider injecting the problematic bean as an `InjectableInstance` (for example `@Inject InjectableInstance<DataSource> ds`) instead. You can check whether that bean is active using `.getHandle().getBean().isActive().result()` on the `InjectableInstance`, and retrieve the bean instance using `InjectableInstance#get()`. See https://quarkus.io/guides/datasource#datasource-active for an example of such a setup for `DataSource` beans -- the idea would be similar for a `Flyway`/`LiquibaseFactory` beans. == Flyway [[flyway-inactive-inactive-fail-fast]] === Flyway usage fails fast if datasource is deactivated or has no URL set Starting with this version of Quarkus, datasources that are deactivated or have no URL set will lead to a startup failure if Quarkus can detect a `Flyway` bean is used for that datasource. See the <<datasource-inactive-fail-fast,datasources,similar migration guide entry for datasources>> for more information. == Liquibase [[liquibase-inactive-if-no-url]] === Liquibase usage fails fast if datasource is deactivated or has no URL set Starting with this version of Quarkus, datasources that are deactivated or have no URL set will lead to a startup failure if Quarkus can detect a `LiquibaseFactory` bean is used for that datasource. See the <<datasource-inactive-fail-fast,datasources,similar migration guide entry for datasources>> for more information.