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

Class loader issues in AvroRpcTest with Quarkus 2.0.0.Alpha3 #2651 #2859

Merged
merged 1 commit into from Jun 30, 2021

Conversation

JiriOndrusek
Copy link
Contributor

fixes #2651
supersedes #2845

Adds support of new SPI - apache/camel#5714

This enhancement uses vertx for the http server. Servers are started for required ports ad are managed via quarkus.
Tests are still using jetty http server - but only in scope test.

Unfortunatelly there is no public access to these methods (which have to be used in http server) - HttpTranscier#readBuffers, HttpTransciever#writeBuffers and HttpTransciever#writeLength. For that reason, vertx request handler has to leverage RespondServlet, which requires creation of very simple HttpServletRequest and HttpServletResponse, which basically wraps byte[].
It makes sense to ask whether it is possible to make these methods public (or for example through creations of class called VertxResponder). If yes, VertxHttpServer.java could be simplified - but that would be covered by follow-up issue.

@ppalaga It seems, that we don't need to use any blocking feature of vertx here, camel component waits for the response of the server. But there is a small issue here. Vertx logs warnings if thread is blocked for more than 2000 ms. I'm not sure whether this could affect users. (but it doesn't affect the functionality) WDYT?

@JiriOndrusek JiriOndrusek changed the title working Class loader issues in AvroRpcTest with Quarkus 2.0.0.Alpha3 #2651 Jun 29, 2021
@ppalaga
Copy link
Contributor

ppalaga commented Jun 29, 2021

@JiriOndrusek is there any reason why this PR is against camel-main? Wouldn't main be fine too?

@JiriOndrusek JiriOndrusek changed the base branch from camel-main to main June 29, 2021 15:24
@JiriOndrusek
Copy link
Contributor Author

@ppalaga yes, you are right. I forgot that the current main already uses camel 3.11. I rebased PR.

Copy link
Contributor

@ppalaga ppalaga left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks quite elegant!

Tests are still using jetty http server - but only in scope test.

I cannot find the scope change for the jetty artifact. Is it there?

Unfortunatelly there is no public access to these methods (which have to be used in http server) - HttpTranscier#readBuffers, HttpTransciever#writeBuffers and HttpTransciever#writeLength. For that reason, vertx request handler has to leverage RespondServlet, which requires creation of very simple HttpServletRequest and HttpServletResponse, which basically wraps byte[].
It makes sense to ask whether it is possible to make these methods public (or for example through creations of class called VertxResponder). If yes, VertxHttpServer.java could be simplified - but that would be covered by follow-up issue.

+1 this PR is OK for now, we can ask Avro to change the visibility and improve our code once they do. Actually when asking for some changes there, an avro-ipc artifact without the Servlet API dependency would be nice.

@ppalaga It seems, that we don't need to use any blocking feature of vertx here, camel component waits for the response of the server. But there is a small issue here. Vertx logs warnings if thread is blocked for more than 2000 ms.

My understanding is that ResponderServlet.service() entails the execution of the whole Camel route. We never know whether the code in the route is or is not going to block. Hence we have to handle it as blocking. @davsclaus may want to correct me if I am wrong.

@jamesnetherton
Copy link
Contributor

We never know whether the code in the route is or is not going to block

Yeah - we had to deal with the same problem in the platform-http consumer. So we process the request on a separate thread with vertx.executeBlocking.

@JiriOndrusek
Copy link
Contributor Author

Follow-up issue #2861
(in case that avro brings a better API for this case - https://issues.apache.org/jira/browse/AVRO-3166)

Copy link
Contributor

@ppalaga ppalaga left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me. @jamesnetherton would you like to throw a final look?

I have filed #2862 for the OpenStack failure

Copy link
Contributor

@jamesnetherton jamesnetherton left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks ok to me, so let's merge as-is.

I still think there's potentially some room to optimise things a bit, but I need to dig into the Quarkus Vert.x internals before creating any follow-up issues.

@ppalaga ppalaga merged commit 5967ae2 into apache:main Jun 30, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Class loader issues in AvroRpcTest with Quarkus 2.0.0.Alpha3
3 participants