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

Fix dev service always restarting on named datasource configuration change #41736

Merged
merged 1 commit into from
Jul 22, 2024

Conversation

Malandril
Copy link
Contributor

@Malandril Malandril commented Jul 6, 2024

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, the cachedProperties 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:

quarkus.datasource."named".db-kind=mariadb

cachedProperties will cache the following entries as set by setDataSourceProperties:

quarkus.datasource."named".db-kind=mariadb
quarkus.datasource.named.db-kind=mariadb

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.

Copy link
Member

@gsmet gsmet left a 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

@radcortez
Copy link
Member

I'm not sure why we need to have both versions,but apparently there was another issue: See #26815.

@Malandril
Copy link
Contributor Author

The last commit actually seems to break the build on the pipelines.

In the jpa-postgresql integration test.
A java.util.NoSuchElementException: SRCFG00011: Could not expand value postgres.url in property quarkus.datasource.jdbc.url

As apparently when creating the cached properties, the postgres.url is not available.
This does not happen when i run the test myself.

Copy link
Member

@yrodiere yrodiere left a 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.

@Malandril Malandril force-pushed the fix-dev-db-restart branch 2 times, most recently from 8f78722 to 6a7e471 Compare July 14, 2024 10:44
Copy link
Member

@yrodiere yrodiere left a 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.

@quarkus-bot

This comment has been minimized.

Copy link
Member

@yrodiere yrodiere left a 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.

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
@yrodiere yrodiere added the triage/waiting-for-ci Ready to merge when CI successfully finishes label Jul 22, 2024
@quarkus-bot

This comment has been minimized.

@quarkus-bot
Copy link

quarkus-bot bot commented Jul 22, 2024

Status for workflow Quarkus CI

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

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

@Malandril
Copy link
Contributor Author

Thanks for the review and the help @yrodiere @radcortez

@radcortez
Copy link
Member

Thanks for the review and the help @yrodiere @radcortez

No. Thank you for your patience :)

@yrodiere yrodiere merged commit 1748f93 into quarkusio:main Jul 22, 2024
42 checks passed
@quarkus-bot quarkus-bot bot removed the triage/waiting-for-ci Ready to merge when CI successfully finishes label Jul 22, 2024
@quarkus-bot quarkus-bot bot added this to the 3.14 - main milestone Jul 22, 2024
@yrodiere yrodiere changed the title Prevent restart when named datasources are quoted, or unquoted Jul 22, 2024
@yrodiere yrodiere changed the title Fix dev service always restart on named datasource configuration change Jul 22, 2024
@yrodiere
Copy link
Member

Merged, thanks a lot!

@yrodiere
Copy link
Member

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

@gsmet gsmet modified the milestones: 3.14 - main, 3.13.0 Jul 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment