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

Use SmallRyeConfigBuilder Customizer to generate Quarkus configuration #35322

Merged
merged 1 commit into from
Oct 6, 2023

Conversation

radcortez
Copy link
Member

@radcortez radcortez commented Aug 11, 2023

These are significant changes, with the following goals:

  • Simplify the generation of Config, by adding a Customizer and removing all individual components registration in the generator
  • Register components directly and avoid ServiceLoader
  • Merge Quarkus defaults source into SmallRye Config built-in defaults

Used config-quickstart to measure improvements with Java 17 Temurin and Graal 22.3 - Java 17:

  JVM RSS Native RSS JVM Start Native Start
3.4.0 153403 25714 0.624 0.025
#35322 146538 22543 0.597 0.017
Diff to 3.4.0 -4.68% -14.06% -4.57% -46.09%

Overall, this PR, should show startup time and RSS improvements for every Quarkus application (at scale), since the improvements affect the core, which always executes regardless of which extensions are available.

@quarkus-bot quarkus-bot bot added area/config area/core area/dependencies Pull requests that update a dependency file area/testing labels Aug 11, 2023
@radcortez radcortez force-pushed the config-builder branch 2 times, most recently from f9d2248 to 70ab0ee Compare September 25, 2023 22:23
@radcortez radcortez marked this pull request as ready for review September 25, 2023 22:23
@quarkus-bot

This comment has been minimized.

@quarkus-bot

This comment has been minimized.

Copy link
Contributor

@franz1981 franz1981 left a comment

Choose a reason for hiding this comment

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

Quickly taken a look, great Roberto!

@quarkus-bot

This comment has been minimized.

@quarkus-bot

This comment has been minimized.

@quarkus-bot

This comment has been minimized.

@quarkus-bot

This comment has been minimized.

@gastaldi
Copy link
Contributor

gastaldi commented Sep 27, 2023

@radcortez it seems you're using a Mac, which does not provide a case sensitive volume by default. See the discussion in Zulip and #36180 (comment)

@radcortez
Copy link
Member Author

@radcortez it seems you're using a Mac, which does not provide a case sensitive volume by default. See the discussion in Zulip and #36180 (comment)

When I did a rebase, I noticed that file, but I thought it was just code-style formatting applied by the build.

@quarkus-bot

This comment has been minimized.

@quarkus-bot

This comment has been minimized.

@quarkus-bot
Copy link

quarkus-bot bot commented Oct 3, 2023

✔️ 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.

@@ -23,7 +22,6 @@ public interface ConfigConfig {
/**
* Profile that will be active when Quarkus launches.
*/
@WithDefault("prod")
Copy link
Member

Choose a reason for hiding this comment

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

I can see how we wouldn't want a default for an Optional but do you confirm this doesn't change our behavior?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, it is fine because the profile was always being overridden by the launch mode:
https://github.com/quarkusio/quarkus/pull/35322/files#diff-11168005e2c503c75c2ff0f927c06e4ca8fb9676b91d58e37ea98b05d68cc27dR29

So, there's no point in setting it here. Also, it would show up as the default in the docs, and because the profile changes depending on the launch mode, it doesn't make sense either to show a default profile in there.

Copy link
Member

Choose a reason for hiding this comment

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

Thanks for confirming! Let's get this in then.

@gsmet gsmet merged commit 94abdd4 into quarkusio:main Oct 6, 2023
51 checks passed
@quarkus-bot quarkus-bot bot added the kind/enhancement New feature or request label Oct 6, 2023
@quarkus-bot quarkus-bot bot added this to the 3.5 - main milestone Oct 6, 2023
rsvoboda added a commit to rsvoboda/quarkus-test-suite that referenced this pull request Oct 9, 2023
mjurc added a commit to quarkus-qe/quarkus-test-suite that referenced this pull request Oct 9, 2023
rsvoboda added a commit to rsvoboda/beefy-scenarios that referenced this pull request Oct 10, 2023
mjurc added a commit to quarkus-qe/beefy-scenarios that referenced this pull request Oct 10, 2023
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.

Extension config for both build and run time with same prefix Allow to create runtime only ConfigSource
4 participants