-
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
Rest Client Reactive: SmallRye Stork integration #19998
Rest Client Reactive: SmallRye Stork integration #19998
Conversation
a599860
to
caa3d39
Compare
...asy-reactive/rest-client-reactive/deployment/src/test/resources/stork-application.properties
Show resolved
Hide resolved
...asy-reactive/rest-client-reactive/deployment/src/test/resources/stork-application.properties
Show resolved
Hide resolved
...asy-reactive/rest-client-reactive/deployment/src/test/resources/stork-application.properties
Outdated
Show resolved
Hide resolved
stork.hello-service.service-discovery.1=${quarkus.http.host}:${quarkus.http.test-port} | ||
stork.hello-service.service-discovery.2=localhost:8766 | ||
stork.hello-service.load-balancer=stat-based | ||
hello2/mp-rest/url=stork://hello-service/hello |
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.
Unrelated to stork, but I still find the MP Rest client format odd, especially when used with the more regular format.
extensions/resteasy-reactive/rest-client-reactive/runtime/pom.xml
Outdated
Show resolved
Hide resolved
...time/src/main/java/org/jboss/resteasy/reactive/client/handlers/ClientSendRequestHandler.java
Show resolved
Hide resolved
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.
LGTM
...asy-reactive/rest-client-reactive/deployment/src/test/resources/stork-application.properties
Show resolved
Hide resolved
I'll try and take a look tomorrow. Has it been tested in native? |
No, I'll add a native test, and if I can a dev mode test too |
:+1 |
...time/src/main/java/org/jboss/resteasy/reactive/client/handlers/ClientSendRequestHandler.java
Outdated
Show resolved
Hide resolved
...time/src/main/java/org/jboss/resteasy/reactive/client/handlers/ClientSendRequestHandler.java
Outdated
Show resolved
Hide resolved
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.
Some mention of Stork will also need to appear in the reference documentation that you need to write for the REST Client :)
sure thing |
1dac7b7
to
b4610e5
Compare
This workflow status is outdated as a new workflow run has been triggered. Failing Jobs - Building b4610e5
Failures⚙️ Initial JDK 11 Build #- 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 7 more 📦 extensions/resteasy-reactive/rest-client-reactive/deployment✖ |
b4610e5
to
490c935
Compare
This workflow status is outdated as a new workflow run has been triggered. Failing Jobs - Building 490c935
Full information is available in the Build summary check run. Failures⚙️ Gradle Tests - JDK 11 #- Failing: integration-tests/gradle
📦 integration-tests/gradle✖
✖
✖
✖
✖
✖
⚙️ 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 6 more 📦 extensions/resteasy-reactive/rest-client-reactive/deployment✖
✖
✖
⚙️ JVM Tests - JDK 11 Windows #- Failing: extensions/vertx-http/deployment
! Skipped: core/test-extension/deployment docs extensions/agroal/deployment and 286 more 📦 extensions/vertx-http/deployment✖
✖
✖
✖
✖
✖
✖
✖
✖
✖
✖
⚙️ JVM Tests - JDK 16 #📦 extensions/resteasy-reactive/rest-client-reactive/deployment✖
✖
|
490c935
to
522b003
Compare
This workflow status is outdated as a new workflow run has been triggered. Failing Jobs - Building 522b003
Full information is available in the Build summary check run. Failures⚙️ Gradle Tests - JDK 11 #- Failing: integration-tests/gradle
📦 integration-tests/gradle✖
⚙️ JVM Tests - JDK 11 Windows #- 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 6 more 📦 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 6 more 📦 extensions/resteasy-reactive/rest-client-reactive/deployment✖
✖
|
522b003
to
87611ba
Compare
...ctive-stork/src/test/java/io/quarkus/it/rest/reactive/stork/RestClientReactiveStorkTest.java
Outdated
Show resolved
Hide resolved
...nt/src/main/java/io/quarkus/rest/client/reactive/deployment/RestClientReactiveProcessor.java
Outdated
Show resolved
Hide resolved
@@ -129,13 +160,18 @@ void registerRestClientListenerForTracing( | |||
} | |||
|
|||
@BuildStep | |||
@Record(ExecutionTime.STATIC_INIT) | |||
@Record(ExecutionTime.RUNTIME_INIT) |
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 this change? Can't we just have stork in its own build step that will use the runtime init phase?
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 how much we can do at build time. At least having a Stock build item would be good.
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.
good catch, I need only Stork initialization moved to runtime
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.
@cescoffier what would be the purpose of the stork build item? Mark that initialization is done?
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.
@geoand fixed
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.
Still trying to see what would be possible. Typically, can we check at build time that the configured discovery and load balancing strategy exist.
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.
That would mean that service discovery type has to be build time fixed, no?
I'm not sure we want that
Start to looks good. I wonder what we can handle at build time (see @geoand's comment). I need to think about the configuration, as configuring both stock and the rest-client looks a bit "extra-step-required". |
...asy-reactive/rest-client-reactive/deployment/src/test/resources/stork-application.properties
Show resolved
Hide resolved
@@ -129,13 +160,18 @@ void registerRestClientListenerForTracing( | |||
} | |||
|
|||
@BuildStep | |||
@Record(ExecutionTime.STATIC_INIT) | |||
@Record(ExecutionTime.RUNTIME_INIT) |
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 how much we can do at build time. At least having a Stock build item would be good.
ad2c66c
to
9a09762
Compare
This workflow status is outdated as a new workflow run has been triggered. Failing Jobs - Building 9a09762
Full information is available in the Build summary check run. Failures⚙️ Devtools Tests - JDK 11 #- Failing: integration-tests/devtools
📦 integration-tests/devtools✖
✖
✖
✖
✖
✖
✖
✖
✖
✖
✖
✖
✖
✖
✖
✖
✖
✖
📦 integration-tests/devtools/target/quarkus-codestart-build-test/project-maven-java-c527dbb5-3794-4592-a612-08f511a6450a✖
📦 integration-tests/devtools/target/quarkus-codestart-build-test/project-maven-kotlin-37e22e77-0002-4f35-b0ba-554c05d0e4cf✖
⚙️ Devtools Tests - JDK 11 Windows #- Failing: integration-tests/devtools
📦 integration-tests/devtools✖
✖
✖
✖
✖
✖
✖
✖
✖
✖
✖
✖
✖
✖
✖
✖
✖
✖
📦 integration-tests/devtools/target/quarkus-codestart-build-test/project-maven-java-4b96d3da-f5bd-49d9-ad94-024ec2849b6a✖
📦 integration-tests/devtools/target/quarkus-codestart-build-test/project-maven-kotlin-300ab611-4107-418f-9d39-52351ffc974f✖
⚙️ JVM Tests - JDK 11 #- Failing: integration-tests/spring-web
📦 integration-tests/spring-web✖
⚙️ JVM Tests - JDK 11 Windows #- Failing: integration-tests/spring-web
📦 integration-tests/spring-web✖
⚙️ Native Tests - Spring #- Failing: integration-tests/spring-web
📦 integration-tests/spring-web✖
|
9a09762
to
a2b188c
Compare
This workflow status is outdated as a new workflow run has been triggered. Failing Jobs - Building a2b188c
Full information is available in the Build summary check run. Failures⚙️ JVM Tests - JDK 11 #- Failing: integration-tests/spring-web
📦 integration-tests/spring-web✖
⚙️ JVM Tests - JDK 11 Windows #- Failing: integration-tests/spring-web
📦 integration-tests/spring-web✖
⚙️ Native Tests - Spring #- Failing: integration-tests/spring-web
📦 integration-tests/spring-web✖
|
failures don't look related |
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.
LGTM
a2b188c
to
2f2740b
Compare
This workflow status is outdated as a new workflow run has been triggered. Failing Jobs - Building 2f2740b
Failures⚙️ Initial JDK 11 Build #- 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 7 more 📦 extensions/resteasy-reactive/rest-client-reactive/deployment✖ |
2f2740b
to
992a388
Compare
the MP TCK failure seems unrelated, it fails on the fault tolerance TCK |
992a388
to
1874920
Compare
This workflow status is outdated as a new workflow run has been triggered. Failing Jobs - Building 1874920
Full information is available in the Build summary check run. Failures⚙️ 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 6 more 📦 extensions/resteasy-reactive/rest-client-reactive/deployment✖
⚙️ JVM Tests - JDK 11 Windows #- 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 6 more 📦 extensions/resteasy-reactive/rest-client-reactive/deployment✖
⚙️ JVM Tests - JDK 17 #- 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 6 more 📦 extensions/resteasy-reactive/rest-client-reactive/deployment✖
|
1874920
to
ec20dd3
Compare
No description provided.