-
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
MP REST Client config #17220
MP REST Client config #17220
Conversation
@michalszynkiewicz could you have a look at this one? It looks like a worthwhile addition. |
I can add a test case to this if needed. I wanted to get some feedback or review before working on it more, to make sure that the idea of adding those config parameters is OK. |
Not sure what @radcortez thinks, my only concern would be around the namespacing with the Configuration off that prefix is defined in the MicroProfile REST Client spec. As what is being proposed is not part of the spec, we should probably include them in a different way. |
Thanks for feedback. Would |
That is going to be very confusing. I always thought we should have used proper Quarkus config for this. |
Hum... how about if we also use So in essense:
May probably be worthwhile to define a specific vendor namespace in the MP Rest Client spec. |
I agree it makes sense to have a configuration key that isn't tied to the current MP way of defining REST Client config |
@radcortez do I understand correctly that we would keep the class name prefix for both options? So we would have:
and then also:
|
Yes, that is the proposal. We probably need a few +1 before actually going for it. @michalszynkiewicz what do you think? |
We'd duplicate the |
From @radcortez's message:
I gather the |
Wouldn't be confusing for Quarkus to duplicate all the MP specific config? |
We can just relocate the configuration and have I'm not sure which one is more confusing. If having to use two namespaces, or use But I would like to hear other thoughts. |
I agree that duplicating is probably not a good idea. Just keep the custom properties under the different namespace. |
I think we all agree on the new properties going to the Quarkus namespace, just not about the duplication. We can probably more like this and duplicate them later if needed. BTW, another thought I had about this is, when we do an implementation first, and configurations get moved into the spec, they are renamed, but the implementation always retains the original names. In here we are doing the other way around. So what happens if one of the new configurations ends up in the specification? We are going to have spec only configurations, like we have now, quarkus only configurations, and maybe some that are on both spec and quarkus namespaces, but you will have to guess which ones. |
Another aspect is that we need to replicate these new configurations in the RESTEasy Reactive Client, not just the original client. |
What's the reason for creating a separate namespace than If we want to have a separate thing from If we're going to have custom properties under a new namespace, I'd be in favor of having all the properties under the new namespace, and quarkus-specific namespace always winning with MicroProfile ones if duplicated. IMO, we should then advertise the new namespace only. Pro: we get a consistent way of configuring clients, con: documenting MicroProfile stuff becomes a bit more tricky. I'm really not convinced having a separate namespace is the way to go. But if we go this way, and advertise it for all properties, it may shield us from potential namespace change when/if MicroProfile Rest Client moves to Jakarta Rest |
Thanks for pointing this out. I briefly looked at the reactive client and it looks it tries to appear the same to developer (do everything as you would with the classic rest client), but naturally it uses different tech under the hood (vertx). The vertx HTTP client offers different configuration options from the Resteasy one. E.g. there is no "connectionTTL" property in vertx, but there is the "keepAliveTimeout" property, which I assume provides similar functionality (but I didn't have time to test it yet). Now there the question how to approach this:
From my brief investigation (I might be wrong), only two of the four config options I wanted to expose have equivalent in VertX: connectionTTL ~ keepAliveTimeout The only "very important" attribute of those four is "connectionTTL" / "keepAliveTimeout" (because it's needed to overcome the problem with load balancers/proxies using a shorter timeout), the others are just hitching a ride. So if I wanted to simplify as much as possible, I'm OK with exposing this one, call it "connectionTTL" for both clients and use the same namespace. This might work until we realize another attribute is needed. |
There's another suggestion from @stuartwdouglas on google groups (https://groups.google.com/g/quarkus-dev/c/FtJMcrveIgA):
|
Yes. I agree the chances for it to happen are slim. Maybe we are just adding complexity when there is none.
Using
+1
So do you think we should just put any new things in |
We should definitely not have split namespaces, were some stuff is under MP config and some stuff is under
The beerservice would be something like:
or a meta annotation:
This way we are not just duplicating the MP config, but are offering something that is IMHO much more useful and user friendly. |
@radcortez yes, I might be wrong but it seems to be the solution with least drawbacks to me. @stuartwdouglas what's your opinion on adding the new properties under I like the idea of trying to do better than the spec. I'm not sure though if Side note: grouping might work (although this wasn't probably designed for it) in MP Rest Client with: @RegisterRestClient(configKey="myClient")
public interface MyClient1 {
}
@RegisterRestClient(configKey="myClient")
public interface MyClient1 {
} configured by: myClient/mp-rest/url=http://beer.com |
The only issue I have with adding new properties under mp-rest is that there is potential for conflicts, but that is probably not such a big deal. I think for some of these settings we should be able to set global defaults as well. |
Taking into account the input presented here and also a related issue #17832 (which calls for providing alternative way of configuring rest client and GraphQL, that wouldn't use slash character in config properties), I'm proposing following solution:
This allows for grouping clients (multiple client interfaces sharing the same config) by defining the same config key in the client interfaces: |
a01d8b9
to
00c031d
Compare
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.
Just added a small comment in the documentation, but nothing major.
e80a52d
to
ff2ae38
Compare
This workflow status is outdated as a new workflow run has been triggered. Failing Jobs - Building ff2ae38
Full information is available in the Build summary check run. Failures⚙️ Gradle Tests - JDK 11 Windows #- Failing: integration-tests/gradle
📦 integration-tests/gradle✖
⚙️ JVM Tests - JDK 11 #- Failing: extensions/resteasy-reactive/rest-client-reactive/deployment
! Skipped: docs extensions/oidc-client-reactive-filter/deployment extensions/resteasy-reactive/rest-client-reactive-jackson/deployment and 5 more 📦 extensions/resteasy-reactive/rest-client-reactive/deployment✖
✖
✖
✖
✖
⚙️ JVM Tests - JDK 11 Windows #- Failing: extensions/amazon-lambda/deployment extensions/resteasy-reactive/rest-client-reactive/deployment
! Skipped: docs extensions/amazon-lambda-http/deployment extensions/amazon-lambda-rest/deployment and 13 more 📦 extensions/amazon-lambda/deployment✖
📦 extensions/resteasy-reactive/rest-client-reactive/deployment✖
✖
✖
✖
✖
⚙️ JVM Tests - JDK 16 #- Failing: extensions/resteasy-reactive/rest-client-reactive/deployment
! Skipped: docs extensions/oidc-client-reactive-filter/deployment extensions/resteasy-reactive/rest-client-reactive-jackson/deployment and 5 more 📦 extensions/resteasy-reactive/rest-client-reactive/deployment✖
✖
✖
✖
✖
⚙️ Native Tests - Data3 #- Failing: integration-tests/liquibase
📦 integration-tests/liquibase✖ |
Added some test fixes after rebasing. |
a09f9d2
to
c567f10
Compare
Can you please rebase this to make sure it still passes CI? Sorry it wasn't merged before... |
* removing quotes around class name in a json config, * adding note about standard MP notation being supported.
c567f10
to
cef04d9
Compare
I actually rebased the PR myself. |
Thanks! I will watch the results. |
This exposes following configuration options via the
application.properties
file:(Assume org.acme.rest.client.CountriesService is an MP rest client interface.)
This could help with connection pooling issues like #15383.
AFAIK the only other way to set above properties is via registering a
RestClientBuilderListener
like the one bellow, but I didn't find it documented anywhere. Being able to configure those viaapplication.properties
would be more developer friendly.