-
Notifications
You must be signed in to change notification settings - Fork 2.6k
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
Improve compatibility of the REST Client configuration #42932
Conversation
🎊 PR Preview b702e31 has been successfully built and deployed to https://quarkus-pr-main-42932-preview.surge.sh/version/main/guides/
|
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
The CI failures seem related
…On Sun, Sep 1, 2024, 03:46 quarkus-bot[bot] ***@***.***> wrote:
Status for workflow Quarkus CI
This is the status report for running Quarkus CI on commit e69225d
<e69225d>
.
Failing Jobs
Status Name Step Failures Logs Raw logs Build scan
✖ JVM Tests - JDK 17 Build Failures
<#m_8440690412275905414_user-content-test-failures-job-29519280841> Logs
<https://github.com/quarkusio/quarkus/actions/runs/10649192250/job/29519280841> Raw
logs
<https://github.com/quarkusio/quarkus/commit/e69225ded3de3013345457828305d2eaaf66bfc5/checks/29519280841/logs>
🚧
✖ JVM Tests - JDK 21 Build Failures
<#m_8440690412275905414_user-content-test-failures-job-29519281220> Logs
<https://github.com/quarkusio/quarkus/actions/runs/10649192250/job/29519281220> Raw
logs
<https://github.com/quarkusio/quarkus/commit/e69225ded3de3013345457828305d2eaaf66bfc5/checks/29519281220/logs>
🚧
✖ JVM Tests - JDK 17 Windows Build Failures
<#m_8440690412275905414_user-content-test-failures-job-29519281346> Logs
<https://github.com/quarkusio/quarkus/actions/runs/10649192250/job/29519281346> Raw
logs
<https://github.com/quarkusio/quarkus/commit/e69225ded3de3013345457828305d2eaaf66bfc5/checks/29519281346/logs>
🚧
Full information is available in the Build summary check run
<https://github.com/quarkusio/quarkus/runs/29519854942>.
Failures ⚙️ JVM Tests - JDK 17 #
<#m_8440690412275905414_user-content-test-failures-job-29519280841>
- Failing: extensions/resteasy-classic/resteasy-client/runtime ! Skipped: extensions/amazon-lambda-http/http-event-server extensions/amazon-lambda-rest/rest-event-server extensions/amazon-lambda/event-server and 98 more
📦 extensions/resteasy-classic/resteasy-client/runtime
✖
io.quarkus.restclient.runtime.RestClientBaseTest.testClientSpecificConfigs
line 109 - History
<https://ge.quarkus.io/scans/tests?tests.container=io.quarkus.restclient.runtime.RestClientBaseTest&tests.test=testClientSpecificConfigs>
- More details
<https://github.com/quarkusio/quarkus/runs/29519854942#user-content-test-failure-io.quarkus.restclient.runtime.restclientbasetest-1>
- Source on GitHub
<https://github.com/quarkusio/quarkus/blob/e69225ded3de3013345457828305d2eaaf66bfc5/extensions/resteasy-classic/resteasy-client/runtime/src/test/java/io/quarkus/restclient/runtime/RestClientBaseTest.java#L109>
Argument(s) are different! Wanted:
restClientBuilder.baseUrl(
http://localhost
);
-> at io.quarkus.restclient.runtime.RestClientBaseTest.testClientSpecificConfigs(RestClientBaseTest.java:109)
Actual invocations have different arguments:
restClientBuilder.baseUrl(
http://localhost:8080
------------------------------
⚙️ JVM Tests - JDK 21 #
<#m_8440690412275905414_user-content-test-failures-job-29519281220>
- Failing: extensions/resteasy-classic/resteasy-client/runtime ! Skipped: extensions/amazon-lambda-http/http-event-server extensions/amazon-lambda-rest/rest-event-server extensions/amazon-lambda/event-server and 98 more
📦 extensions/resteasy-classic/resteasy-client/runtime
✖
io.quarkus.restclient.runtime.RestClientBaseTest.testClientSpecificConfigs
line 109 - History
<https://ge.quarkus.io/scans/tests?tests.container=io.quarkus.restclient.runtime.RestClientBaseTest&tests.test=testClientSpecificConfigs>
- More details
<https://github.com/quarkusio/quarkus/runs/29519854942#user-content-test-failure-io.quarkus.restclient.runtime.restclientbasetest-1>
- Source on GitHub
<https://github.com/quarkusio/quarkus/blob/e69225ded3de3013345457828305d2eaaf66bfc5/extensions/resteasy-classic/resteasy-client/runtime/src/test/java/io/quarkus/restclient/runtime/RestClientBaseTest.java#L109>
Argument(s) are different! Wanted:
restClientBuilder.baseUrl(
http://localhost
);
-> at io.quarkus.restclient.runtime.RestClientBaseTest.testClientSpecificConfigs(RestClientBaseTest.java:109)
Actual invocations have different arguments:
restClientBuilder.baseUrl(
http://localhost:8080
------------------------------
⚙️ JVM Tests - JDK 17 Windows #
<#m_8440690412275905414_user-content-test-failures-job-29519281346>
- Failing: extensions/resteasy-classic/resteasy-client/runtime ! Skipped: extensions/amazon-lambda-http/http-event-server extensions/amazon-lambda-rest/rest-event-server extensions/amazon-lambda/event-server and 98 more
📦 extensions/resteasy-classic/resteasy-client/runtime
✖
io.quarkus.restclient.runtime.RestClientBaseTest.testClientSpecificConfigs
line 109 - History
<https://ge.quarkus.io/scans/tests?tests.container=io.quarkus.restclient.runtime.RestClientBaseTest&tests.test=testClientSpecificConfigs>
- More details
<https://github.com/quarkusio/quarkus/runs/29519854942#user-content-test-failure-io.quarkus.restclient.runtime.restclientbasetest-1>
- Source on GitHub
<https://github.com/quarkusio/quarkus/blob/e69225ded3de3013345457828305d2eaaf66bfc5/extensions/resteasy-classic/resteasy-client/runtime/src/test/java/io/quarkus/restclient/runtime/RestClientBaseTest.java#L109>
Argument(s) are different! Wanted:
restClientBuilder.baseUrl(
http://localhost
);
-> at io.quarkus.restclient.runtime.RestClientBaseTest.testClientSpecificConfigs(RestClientBaseTest.java:109)
Actual invocations have different arguments:
restClientBuilder.baseUrl(
http://localhost:8080
------------------------------
Flaky tests - Develocity <https://ge.quarkus.io/scans/tests> ⚙️ JVM Tests
- JDK 17 📦 extensions/resteasy-reactive/rest-client/deployment
✖
io.quarkus.rest.client.reactive.stork.StorkResponseTimeLoadBalancerTest.shouldUseFasterService
- History
<https://ge.quarkus.io/scans/tests?tests.container=io.quarkus.rest.client.reactive.stork.StorkResponseTimeLoadBalancerTest&tests.test=shouldUseFasterService>
- expected: "hello, Alice" but was: "hello, I'm a slow server" -
org.opentest4j.AssertionFailedError
org.opentest4j.AssertionFailedError:
expected: "hello, Alice"
but was: "hello, I'm a slow server"
at io.quarkus.rest.client.reactive.stork.StorkResponseTimeLoadBalancerTest.shouldUseFasterService(StorkResponseTimeLoadBalancerTest.java:58)
at java.base/java.lang.reflect.Method.invoke(Method.java:569)
at io.quarkus.test.QuarkusUnitTest.runExtensionMethod(QuarkusUnitTest.java:499)
at io.quarkus.test.QuarkusUnitTest.interceptTestMethod(QuarkusUnitTest.java:413)
—
Reply to this email directly, view it on GitHub
<#42932 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ABBMDPZFWKTVURMNWUTAQ4DZUJPVDAVCNFSM6AAAAABNOI7TZGVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDGMRTGA4TENZSHE>
.
You are receiving this because your review was requested.Message ID:
***@***.***>
|
e69225d
to
1da0004
Compare
Just a wrong assertion: |
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.
Thanks for the PR! I added some comments.
I must admit that I'm a bit concerned that this config requires so many hacks. But I can't say I understand fully all the requirements so it's just a vague impression.
for (RegisteredRestClient restClient : restClients) { | ||
builder.withDefaultValue("quarkus.rest-client.\"" + restClient.getFullName() + "\".force", "true"); | ||
builder.withDefaultValue("quarkus.rest-client." + restClient.getSimpleName() + ".force", "true"); | ||
if (restClient.getConfigKey() != null) { | ||
builder.withDefaultValue("quarkus.rest-client." + restClient.getConfigKey() + ".force", "true"); | ||
builder.withDefaultValue("quarkus.rest-client.\"" + restClient.getConfigKey() + "\".force", "true"); | ||
} | ||
} |
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.
Why are we doing that and enforcing this value here? I don't understand why we would have to do that.
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.
OK, so I have done my homework about this thing and it seems reasonable.
I wonder though if we should come up with a way to filter it from the Dev UI? Maybe by filtering config that shouldn't appear in the doc? If it's not in the doc, I would thing that we wouldn't want it to be visible.
Or we would need another annotation.
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.
Note that I'm not requesting a change for this patch. Just thinking out loud for the future.
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.
I've tried to add that explanation here:
Lines 19 to 27 in 1da0004
* Usually, named configuration is mapped using a <code>Map</code> because the names are dynamic and unknown to | |
* Quarkus. In the case of the REST Client, configuration names are fixed and known at build time, but not to the point | |
* where the names can be mapped statically, so they still need to be mapped in a <code>Map</code>. | |
* <p> | |
* To populate a <code>Map</code>, because the names are dynamic, the Config system has to rely on the list of | |
* property names provided by each source. This also applies to the REST Client, but since the names are known to | |
* Quarkus, the REST Client configuration could be loaded even for sources that don't provide a list of property | |
* names. To achieve such behaviour, we provide a dummy configuration under each REST Client name to force | |
* the Config system to look up the remaining configuration in the same tree. |
Let me expand that further, and then I can update the javadoc too:
In config, when we populate a Map
, we must rely on the list of property names because the key is dynamic. As an example, if we want to retrieve the list of datasources, we have to look for properties quarkus.datasource."datasource-name"
, and then we do the rest.
There is a catch: of course, this only works when we get at least one property of the tree with the expected name. The problem is that some sources (like Vault) cannot list the properties. In the example case, if all the datasource properties are only set in Vault, we cannot load that configuration because we don't know it exists. What we recommend is to set some base properties and then override them.
In the case of the REST Client, it's a bit of a mix. At the mapping level, we use a Map
because, obviously, we can't represent an unknown number of clients in the mapping structure, but we do know all the possible keys in advance. So, I'm adding a dummy key to force the lookup. Once the Config knows the dynamic part of a key, it will then query all the other pieces directly so that the Vault config will work.
I plan to support this out of the box without having to rely on workarounds.
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.
Yeah, I missed it because of the context of the diff but the Javadoc you already have is perfectly clear. That's what I was trying to say in my previous message by "doing my homework" :)
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.
I wonder though if we should come up with a way to filter it from the Dev UI? Maybe by filtering config that shouldn't appear in the doc? If it's not in the doc, I would thing that we wouldn't want it to be visible.
Yes, it is annoying, and I didn't think of that when I added it in the first place. We could filter it with the doc annotation, or maybe @Deprecated
?
Anyway, I'll add something in the Config system for such cases (we have the same case in OTel), where we can't statically map the configuration, but the keys are well-known. I'll add something that will always query specific keys without relying on the list provided by the sources, so these types of hacks will no longer be required.
/** | ||
* Duplicate mapping of {@link RestClientConfig#url()} to keep a reference of the name used to retrieve the | ||
* <code>url</code>. We need this to reload the <code>url</code> configuration in case it contains an expression | ||
* to <code>${quarkus.http.port}</code>, which is only set after we load the config. | ||
*/ | ||
@ConfigDocIgnore | ||
@WithName("url") | ||
ConfigValue urlValue(); |
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.
Maybe it's a crazy idea but should we affect the random port when reading the config instead? Maybe by using a converter?
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.
Unfortunately, no. The problem here is how we read and set quarkus.http.port
.
One of the first things we do when we start Quarkus is to load the configurations and cache the values in the mappings / roots (this is the same for old roots and new mappings). This is fine when we explicitly set the quarkus.http.port
since the value gets expanded to the explicitly set port.
When we set a random port of 0
, we delegate the random port assignment to Vert.x, but we also need the configuration to be loaded to start Vert.x. So we have a chicken-egg problem here because the configuration starts before Vert.x, any expansion done for ${quarkus.http.port}
is 0
(the random port). This will get cached in the config objects.
Once Vert.x sets the port, this is mutating the SystemProperties
:
Lines 1312 to 1332 in f399671
if (https) { | |
actualHttpsPort = actualPort; | |
validateHttpPorts(actualHttpPort, actualHttpsPort); | |
} else { | |
actualHttpPort = actualPort; | |
validateHttpPorts(actualHttpPort, actualHttpsPort); | |
} | |
if (actualPort != options.getPort()) { | |
// Override quarkus.http(s)?.(test-)?port | |
String schema; | |
if (https) { | |
clearHttpsProperty = true; | |
schema = "https"; | |
} else { | |
clearHttpProperty = true; | |
actualHttpPort = actualPort; | |
schema = "http"; | |
} | |
portSystemProperties = new PortSystemProperties(); | |
portSystemProperties.set(schema, actualPort, launchMode); | |
} |
Which is a different problem that is causing other issues like:
#42844
#42106 (comment)
#27996
When we get the actual port, all the configuration objects are cached, and we need to query the configuration again to get the updated value. The problem is getting the discovered property name by the REST Client (see explanation in #42904 (comment)). Adding a ConfigValue
here at the mapping class allows us to keep a reference to the full property name of the `URL' so we can query it without having to reconstruct all the REST Client name variations.
This worked previously because it was specifically implemented in REST Client with:
#21530
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.
But is Vert.x doing things that we can't do ourselves when the config is read? The contract of this is that you get a random port, we don't really care if it's Vert.x assigning it? Especially since you might want a random port for something else than Vert.x.
Now maybe Vert.x is doing something very clever that we can't replicate easily at the config level?
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.
BTW, if it wasn't obvious, I'm not advocating that we change things now. But I'm wondering if it's worth discussing it for the future.
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.
Also obviously, we would need to keep a list of the ports randomly affected around to make sure we don't affect the same port twice.
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.
I fully agree. We need to remove the random port generation from Vert.x and do it before the config is set up. This would eliminate many of our current hacks and issues.
Of course, I didn't want to make that change here, so I had to rely on another hack :)
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Yes, I get it :( The reason why we have to come up with these many hacks was how the REST Client configuration was implemented: Namely, ignoring all the rules set in place by config roots. We probably shouldn't have allowed that, but at the time, we didn't have mappings fully developed and we didn't encounter issues like #41313, #42092 and others we tried to resolve with #42106. Moving forward, I'll see what I can add to work out of the box in the Config system directly to be able to remove some of these hacks. |
I'm fully in favor of removing the hacks at some point in the near future. |
That is the intention, especially because it will unlock other features, fix bugs, and remove other hacks. During this work, I took a lot of notes, and I'll start filling in issues with the work we have to do. |
Have you by any chance done any profiling for this change? Asking because lately we've seen stuff creep up where we didn't expect it :) |
1da0004
to
39d5ba1
Compare
Status for workflow
|
Status for workflow
|
No. My concern was making it compatible with the old implementation, but both have serious issues. Currently, to populate a REST Client Config, we may have to query all of the following names:
This is crazy! Granted, that We probably need to break a few things to improve this... for instance if I set quarkus.rest-client."foo.bar.Client".url=
quarkus.rest-client."Client".uri=
quarkus.rest-client.config-key.providers=
quarkus.rest-client."config-key".connect-timeout=
config-key/mp-rest/read-timeout= I hope no one is mixing things like this :) I think we should move forward with some things (breaking), like:
I still have to think about quotes... currently, the default implementation in |
+1 from me. But we need to be careful as it would be a breaking change. But that's totally something we could merge for 3.16. |
Agreed, go ahead and break/remove so of the old stuff for 3.16 |
Also, I think we can drop most of the fallbacks we introduced recently (including My rule of thumb is that we need the deprecation to be in at least one LTS and not be too new. |
Makes sense.
Kill them :) |
Sharing some numbers (without much analysis yet - single REST Client app with
This does not measure isolated code changes (it was just to get some quick numbers), so we cannot be sure of the actual degradation. When comparing startup performance, we expect some degradation between With this PR, we can also expect to get worse startup numbers since we are doing even more work. Memory-wise, after the initial request, things are very similar (and I still need to try this PR directly on top of #42106 for a fair comparison. I'm not too fond of these startup numbers, but I'm also unsure if we can improve them without breaking things. I can see two options:
In the meantime, I'll see if I can improve it in any way. |
Stuck between a rock and a hard place :) |
I guess going with an extra hack for the backport and then making the proper fix for |
I think we can live with this and merge this. I think it's better than reverting everything now. |
I very much look forward to having this cleaned up for 3.16 though :) |
Yes, this is going to be a top priority. |
The move of REST Client configuration to
@ConfigMapping
in #42106 is causing some issues that this PR should fix:The old config was only queried when the REST Client was being created, so it was possible to use expressions like
${quarkus.http.port}
in the URL/ URI. For all extensions, the config is loaded when the application starts, so any further changes to the configuration are not visible. This is the case when we generate a random port and update${quarkus.http.port}
. We are now loading the URL / URI on REST Client creation.The
force
lookup to allow loading configuration from "invisible" sourcesquarkus/extensions/resteasy-classic/rest-client-config/runtime/src/main/java/io/quarkus/restclient/config/AbstractRestClientConfigBuilder.java
Lines 19 to 27 in e69225d
@RegisterRestClient#configKey
had multiple segments, so it was causing a few issues.Also, the old implementation allowed the mixing of quoted and unquoted keys in the configuration names. This was never supported in the current Config system. I've added it here, using some interceptors and I'll properly implement this in
SmallRyeConfig
directly so other extensions behave the same way.Fixes quarkus-rest-client-jackson - Force property #42857
Fixes Test:
quarkus.http.port
is not updated with random port activated throughquarkus.http.test-port=0
#42944Fixes https://quarkusio.zulipchat.com/#narrow/stream/187030-users/topic/Rest-client.20url.20is.20now.20build.20time.20property.3F