-
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
Move core configuration to @ConfigMapping #44079
Conversation
/cc @brunobat (opentelemetry), @gsmet (elasticsearch), @loicmathieu (elasticsearch), @marko-bekhta (elasticsearch), @yrodiere (elasticsearch) |
🙈 The PR is closed and the preview is expired. |
5c95a2c
to
a3f69aa
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
@radcortez the failures look related. BTW, maybe we could tackle #43149 at the same time, while we're at it? |
Yes, I was already expecting something to fail, and I also wanted to implement #43149, which is why I added "not merge" in the description. I'll complete this in the meantime. |
Alternatively, we can hold the move of the related configuration, but I would prefer to disable the test :) |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
@radcortez I am on the road today, but I plan to test this PR and report back any potential changes in binary size (probably tomorrow or the day after). As discussed in #39748 (comment), the changes are not expected to have a huge impact but it's worth having a look before moving forward. Thank you. For reference, this should also help us with #44312 |
Sure. I'm also taking some measurements of RSS and startup time. There is a slight increase, but these are related to some additional checks that I'll address in a separate PR. |
@radcortez I compared the results from a run of this PR (3f69fd3) versus a run based on 901675f In terms of binary size I see that:
In terms of RSS usage at build time I see mixed results (which I attribute to noise) so I can't draw a conclusion out of them. The largest increase (10% or 330 MB) appears to be in |
Thanks! I guess that a slight increase in the image size may occur due to the following factors:
So compared to the old approach, we are adding x2 classes, and each class contains and Regarding the RSS usage, I really have no idea what would be the cause. We are now extensively using mappings in a lot of other places and we never had such problem. |
Status for workflow
|
I think it would still be interesting to get a diff of the list of classes in the case of |
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.
Let's merge this as it's where we want to go.
We will need to make sure we improve things as much as possible though.
Milestone is already set for some of the items: We haven't automatically updated the milestones for these items. This message is automatically generated by a bot. |
@ConfigRoot
and@ConfigMapping
#42114quarkus.configuration.build-time-mismatch-at-runtime
should be in thequarkus.config
namespace #43149We still miss
ClassLoadingConfig
, which needs to be handled separately.Please merge the following PRs first to address a couple of performance issues: