-
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
Update to Vertx 4.5.1 #38034
Update to Vertx 4.5.1 #38034
Conversation
This comment has been minimized.
This comment has been minimized.
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 asked a couple of questions.
integration-tests/oidc/src/main/java/io/quarkus/it/keycloak/VertxResource.java
Outdated
Show resolved
Hide resolved
@@ -29,7 +29,7 @@ public class KeycloakXTestResourceLifecycleManager implements QuarkusTestResourc | |||
private static String KEYCLOAK_SERVER_URL; | |||
private static final String KEYCLOAK_REALM = "quarkus"; | |||
private static final String KEYCLOAK_SERVICE_CLIENT = "quarkus-service-app"; | |||
private static final String KEYCLOAK_VERSION = System.getProperty("keycloak.version"); | |||
private static final String KEYCLOAK_VERSION = System.getProperty("keycloak.version", "23.0.1"); |
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'm not sure how this property was supposed to be passed before? @sberyozkin ?
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.
Without you cannot run the tests from the IDE. It super inconvenient when you need to debug dozens of tests.
I can revert that change, but you can be sure I will redo the same next time I need to debug these tests.
@Test | ||
@Disabled("Does not work with Vert.x 4.5.1 - to be investigated") | ||
public void testPostTextWithServlet() throws Exception { | ||
testPostTextByEvent("/servlet/hello"); | ||
testPostText("/servlet/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.
What's the plan for these?
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.
Well, the plan is to dive into the depth of undertow and try to understand why the handler is not called anymore. It only happens when used with the virtual server, which I hope to remove at some point (as it breaks every single time).
I already tried to debug it without luck and we need to make progress to meet the deadline.
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.
Agreed, thanks for the details.
...y-http/runtime/src/main/java/io/quarkus/funqy/runtime/bindings/http/VertxRequestHandler.java
Outdated
Show resolved
Hide resolved
Also, great work :). |
d551c08
to
8318cf5
Compare
import io.vertx.core.Vertx; | ||
import io.vertx.core.VertxOptions; | ||
import io.vertx.core.WorkerExecutor; | ||
import io.vertx.core.*; |
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.
Sorry I missed that but we don't want star imports.
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 except for the star imports. Sorry for noticing them so late.
This comment has been minimized.
This comment has been minimized.
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 got rid of the star imports, rebased and force pushed.
Let's wait for CI and merge.
Ah darn, there is now a conflict in the bom :( |
I'll fix it once I'm done with my current task. |
This class has been removed. The worker context class has also been removed (used in test). We now use the ContextInternal class.
The createMessage method signature changed.
…n handler. As the body may have already been read.
…a context after the application shutdown.
It avoids a race condition when reading the body
This version contains the API for Vert.x 4.5.1. Note that there is a few change in the Redis low level API. These changes are backward compatible.
This version has been built with Vert.x 4.5.1 and Netty 4.1.103.Final.
This version is using Vert.x 4.5.1 and the Mutiny bindings 3.8.0
This is the version required to work with Vert.x 4.5.1 (due to the API changes in this Vert.x release)
It avoids a potential race condition trying to read the body twice.
When using Undertow, the request body is not read correctly. Something is wrong between Undertow, Quarkus HTTP and the virtual server.
Done! |
✔️ The latest workflow run for the pull request has completed successfully. It should be safe to merge provided you have a look at the other checks in the summary. You can consult the Develocity build scans. |
Update to Vert.x 4.5.1 and Netty 4.1.103.Final
\CC @Sanne