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

Move http configuration to @ConfigMapping #45769

Draft
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

radcortez
Copy link
Member

@radcortez radcortez commented Jan 21, 2025

Copy link

quarkus-bot bot commented Jan 21, 2025

/cc @brunobat (micrometer), @ebullient (metrics,micrometer), @jmartisk (metrics)

This comment has been minimized.

This comment has been minimized.

@gsmet
Copy link
Member

gsmet commented Jan 22, 2025

Nice! A few comments.

2025-01-22T00:58:57.1850519Z [ERROR] Failed to execute goal org.codehaus.mojo:exec-maven-plugin:3.1.0:java (generate-doc-reference-index) on project quarkus-documentation: An exception occurred while executing the Java class. Duplicate key all-config.adoc#quarkus-vertx-http_quarkus-http-root-path (attempted merging values Configuration property documentation and Configuration property documentation) -> [Help 1]

You need to mark the legacy config as ignored with @ConfigDocIgnore.

Also, didn't we agree that it should be something we merge after the next LTS is branched? I don't mind having a PR, it's cool but should we make it draft until mid-February?
If you changed your mind, can we discuss it?

@radcortez
Copy link
Member Author

You need to mark the legacy config as ignored with @ConfigDocIgnore.

Ah yes, I'll do that.

Also, didn't we agree that it should be something we merge after the next LTS is branched? I don't mind having a PR, it's cool but should we make it draft until mid-February? If you changed your mind, can we discuss it?

Yes, we don't need to merge it now; we can put it in draft. I had the code here to test the performance stuff, and I think it is in good shape now. Also, there are a lot of conflicts every time I need to rebase, so I wanted to keep updating it in small increments instead of waiting weeks until we need to do it. With the PR, it is easier to check the conflicts and fix them.

@radcortez radcortez marked this pull request as draft January 22, 2025 10:36
@gsmet
Copy link
Member

gsmet commented Jan 22, 2025

That being said, the advantage of merging it now is that extensions will be able to adapt to inject the new interface while still supporting 3.20 LTS.

So maybe let's think about it a bit more. And yes, testing things are good performance wise is important if we finally decide to include this in 3.19.

@radcortez
Copy link
Member Author

Sure.

Copy link

🎊 PR Preview 32144de has been successfully built and deployed to https://quarkus-pr-main-45769-preview.surge.sh/version/main/guides/

  • Images of blog posts older than 3 months are not available.
  • Newsletters older than 3 months are not available.

@michalvavrik
Copy link
Member

I think it's little bit strange to make config classes effectively deprecated with #45781 and not merging this.

If you agreed to do this after next LTS, can you point me to the discussion? AFAIK I am watching all the channels, I even opened Quarkus issue so I'd expect to mention. I am interested in reasons because this is something I have been waiting for a long time. I am in no hurry, I'll probably not start on it right now anyway, just curious WHY because this feels like I don't see big picture.

@gsmet
Copy link
Member

gsmet commented Jan 22, 2025

Hey @michalvavrik ,

I think it's little bit strange to make config classes effectively deprecated with #45781 and not merging this.

Not really, the deprecation is here to draw attention on the fact that they will be dropped at some point.
It doesn't mean nobody is using it.
Both in Quarkus and in the broader extension ecosystem, we have and will have for quite some time extensions using this now deprecated infrastructure.

If you agreed to do this after next LTS, can you point me to the discussion? AFAIK I am watching all the channels, I even opened Quarkus issue so I'd expect to mention. I am interested in reasons because this is something I have been waiting for a long time. I am in no hurry, I'll probably not start on it right now anyway, just curious WHY because this feels like I don't see big picture.

It was a private discussion with Roberto (sorry!). As I rebased the original PR during the week between Christmas and end of year and Roberto had told me he was also making progress on this, I asked him if he wanted me to close my PR (#45274).
We then talked about the schedule and if it was a good thing to have it so late before the LTS or not.

You have a point that this discussion should have been public.

There are several reasons why we are being extra cautious with Vert.x HTTP (while merging other PRs doing the exact same thing on other extensions):

  • the Vert.x HTTP config is (unfortunately) consumed by several extensions outside of Quarkus - which means we needed a compatibility layer - which we now have in this PR but we have no idea if it's enough and it might take time to make sure things are fine for everyone.
  • we had some performance issues with this particular PR, probably because a lot of properties. As this extension is central to Quarkus and probably used by 99% of the applications, we need to be extra careful.

These two reasons caused us to revert the whole PR in the past: we merged it, then saw the issues and reverted it.

I hope it clarifies why we are being extra cautious about Vert.x HTTP. Don't hesitate to ask for clarifications if still unclear in some areas.

@michalvavrik
Copy link
Member

Not really, the deprecation is here to draw attention on the fact that they will be dropped at some point.
It doesn't mean nobody is using it.
Both in Quarkus and in the broader extension ecosystem, we have and will have for quite some time extensions using this now deprecated infrastructure.

Ok, I didn't consider this. Thanks

These two reasons caused us to revert the whole PR in the past: we merged it, then saw the issues and reverted it.

yes, that's what I was looking for. Thank you

I hope it clarifies why we are being extra cautious about Vert.x HTTP. Don't hesitate to ask for clarifications if still unclear in some areas.

Don't take me wrong @gsmet , this is not blocking me, I am simply interested in this topic. I didn't mean to make waves. The answer is sufficient.

@gsmet
Copy link
Member

gsmet commented Jan 22, 2025

Don't take me wrong @gsmet , this is not blocking me, I am simply interested in this topic. I didn't mean to make waves. The answer is sufficient.

Oh I didn't. Your questions were very legitimate.

Additional info: I'm working on an ADR to try to schedule how we could phase this thing out. Today was a bit too chaotic for me to finish it but I hope I'll be able to push a first version for discussion tomorrow.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment