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

Don't include non-configured data sources in health checks #21003

Merged
merged 1 commit into from
Nov 2, 2021

Conversation

jmartisk
Copy link
Contributor

Fixes #20950

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! I added a few suggestions!

Set<String> names = support.getConfiguredNames();
Set<String> excludedNames = support.getExcludedNames();
for (String name : names) {
DataSource ds = name.equals(DataSourceUtil.DEFAULT_DATASOURCE_NAME)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please use DataSourceUtil.isDefault().

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

boolean isDefault = DEFAULT_DS.equals(dataSource.getKey());
try (Connection con = dataSource.getValue().getConnection()) {
boolean valid = con.isValid(0);
boolean isDefault = DataSourceUtil.DEFAULT_DATASOURCE_NAME.equals(dataSource.getKey());
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please use DataSourceUtil.isDefault().

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

for (String name : names) {
DataSource ds = name.equals(DataSourceUtil.DEFAULT_DATASOURCE_NAME)
? (DataSource) Arc.container().instance(DataSource.class).get()
: (DataSource) Arc.container().instance(name).get();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you use instance(Class<T> type, Annotation... qualifiers) instead? You have DataSourceLiteral for this purpose.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

@quarkus-bot
Copy link

quarkus-bot bot commented Oct 26, 2021

This workflow status is outdated as a new workflow run has been triggered.

Failing Jobs - Building b836d84

Status Name Step Failures Logs Raw logs
Initial JDK 11 Build Build Failures Logs Raw logs

Failures

⚙️ Initial JDK 11 Build #

- Failing: extensions/agroal/runtime 
! Skipped: devtools/bom-descriptor-json docs extensions/agroal/deployment and 137 more

📦 extensions/agroal/runtime

Failed to execute goal net.revelc.code:impsort-maven-plugin:1.6.2:check (check-imports) on project quarkus-agroal: Imports are not sorted in /home/runner/work/quarkus/quarkus/extensions/agroal/runtime/src/main/java/io/quarkus/agroal/runtime/health/DataSourceHealthCheck.java

@geoand geoand requested a review from gsmet October 26, 2021 13:36
@gsmet gsmet merged commit 9873f06 into quarkusio:main Nov 2, 2021
@quarkus-bot quarkus-bot bot added this to the 2.5 - main milestone Nov 2, 2021
@jmartisk jmartisk deleted the main-issue-20950 branch November 2, 2021 09:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Quarkus Agroal Health Check: problem with additional unnamed DataSource producers
2 participants