-
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
Fix dev service always restarting on named datasource configuration change #41736
Conversation
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.
Thanks for the PR! That's indeed an annoying issue.
TBH, I'm not entirely sure why we store the two versions in cachedProperties
. Maybe to avoid triggering a reload if only the quoting changed. But yeah, breaking the common case for it doesn't make a lot of sense.
I wonder if we could find a better way to do that as it looks like a (useful!) hack on top a hack.
/cc @radcortez @yrodiere
I'm not sure why we need to have both versions,but apparently there was another issue: See #26815. |
...c/main/java/io/quarkus/datasource/deployment/devservices/DevServicesDatasourceProcessor.java
Outdated
Show resolved
Hide resolved
...c/main/java/io/quarkus/datasource/deployment/devservices/DevServicesDatasourceProcessor.java
Outdated
Show resolved
Hide resolved
...c/main/java/io/quarkus/datasource/deployment/devservices/DevServicesDatasourceProcessor.java
Show resolved
Hide resolved
...c/main/java/io/quarkus/datasource/deployment/devservices/DevServicesDatasourceProcessor.java
Show resolved
Hide resolved
The last commit actually seems to break the build on the pipelines. In the As apparently when creating the cached properties, the |
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.
SmallRyeConfig#getOptionalValues
seems promising, but I think we got carried away here... see comments below.
...c/main/java/io/quarkus/datasource/deployment/devservices/DevServicesDatasourceProcessor.java
Outdated
Show resolved
Hide resolved
...c/main/java/io/quarkus/datasource/deployment/devservices/DevServicesDatasourceProcessor.java
Show resolved
Hide resolved
...c/main/java/io/quarkus/datasource/deployment/devservices/DevServicesDatasourceProcessor.java
Show resolved
Hide resolved
eb559d8
to
8f78722
Compare
...c/main/java/io/quarkus/datasource/deployment/devservices/DevServicesDatasourceProcessor.java
Outdated
Show resolved
Hide resolved
...c/main/java/io/quarkus/datasource/deployment/devservices/DevServicesDatasourceProcessor.java
Outdated
Show resolved
Hide resolved
...c/main/java/io/quarkus/datasource/deployment/devservices/DevServicesDatasourceProcessor.java
Outdated
Show resolved
Hide resolved
...c/main/java/io/quarkus/datasource/deployment/devservices/DevServicesDatasourceProcessor.java
Show resolved
Hide resolved
8f78722
to
6a7e471
Compare
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.
Thanks, the approach looks good. Some details need our attention though; see comments below.
In particular we'll need @radcortez opinion on that exception.
...c/main/java/io/quarkus/datasource/deployment/devservices/DevServicesDatasourceProcessor.java
Outdated
Show resolved
Hide resolved
...c/main/java/io/quarkus/datasource/deployment/devservices/DevServicesDatasourceProcessor.java
Outdated
Show resolved
Hide resolved
...c/main/java/io/quarkus/datasource/deployment/devservices/DevServicesDatasourceProcessor.java
Outdated
Show resolved
Hide resolved
This comment has been minimized.
This comment has been minimized.
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.
Thanks a lot for your patience, I know it's been a rough ride but it's looking good in the end.
I think the last remaining piece is handling of two runtime properties... see below.
...c/main/java/io/quarkus/datasource/deployment/devservices/DevServicesDatasourceProcessor.java
Outdated
Show resolved
Hide resolved
Simpler property cache using SmallryeConfig Only cache a subset of properties in datasource restart cache Disable expressions when getting datasource properties for restart Create a map cache from dataSourcesBuildTimeConfig Add username and password
611e353
to
d9b4534
Compare
This comment has been minimized.
This comment has been minimized.
Status for workflow
|
Thanks for the review and the help @yrodiere @radcortez |
No. Thank you for your patience :) |
Merged, thanks a lot! |
@gsmet Marking for backport on 3.8 as I think the final implementation would apply cleanly, and the problem looks like something that would impair development significantly. |
When a named datasource is used, quarkus tries to check if a some of the
cachedProperties
have changed to know if it needs to restart the datasource, thecachedProperties
stores the properties with and without quotes.This causes the change check to always be triggered, because when it will compare with the actual microprofile property only one version will exist, either the quoted version, or the unquoted version.
For example with the given property:
cachedProperties
will cache the following entries as set bysetDataSourceProperties
:And when it will try to check the difference only the
quarkus.datasource."named".db-kind=mariadb
will be present and so the restart will be triggered.This PR uses gets a map of the datasource configs which will be used as cache to verify if values have changed.
The drawback of this is that any change in the
quarkus.datasource
properties triggers a restart, which i think is still better than having a non consistent behaviour because of quotes.Bug Reproducer here that i used to test this PR.