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

Move gRPC extension to new Vert.x gRPC impl #28654

Merged
merged 1 commit into from Oct 31, 2022
Merged

Conversation

alesj
Copy link
Contributor

@alesj alesj commented Oct 18, 2022

Added support for new Vert.x client/server based implementation.
It doesn't instantiate and start any new gRPC servers, it re-uses existing HTTP server.
The client side uses own Vert.x based Channel implementation.

New code is split between server and client side (doh!).
Server side simply registers a new route against existing HTTP server, only serving gRPC requests (based on content-type atm). Actual services and interceptors handling stays the same. The new implementation currently needs to be explicitly enabled: quarkus.grpc.server.use-separate-server=false
Client side instantiates a new Vert.x GrpcClient based Channel implementation, handling client closure at the stop.
Interceptors handling stays the same. Not all previous client config could be mapped onto new implementation (TODO?).
The new implementation currently needs to be explicitly enabled: quarkus.grpc.clients.<name>.use-quarkus-grpc-client=true
I guess once we feel comfortable enough, we'll revert the configuration usage for implementation selection.

Almost all (except the one's with load balancing / Stork) gRPC tests now have double "flavour":

  • old gRPC way - with new server
  • new Vert.x server way, with re-using HTTP server

Also added a mixture test - mixing old server with new client, and vice-versa.

New Vert.x client implementation is still missing load balancing and name resolver support.
(hence missing Vert.x client based tests ... a TODO)

@quarkus-bot
Copy link

quarkus-bot bot commented Oct 18, 2022

Thanks for your pull request!

The title of your pull request does not follow our editorial rules. Could you have a look?

  • title should not end up with dot

This message is automatically generated by a bot.

@alesj alesj changed the title Move gRPC extension to new Vert.x gRPC impl. Move gRPC extension to new Vert.x gRPC impl Oct 18, 2022
@quarkus-bot

This comment has been minimized.

@cescoffier
Copy link
Member

@alesj there are various gRPC tests failing, can you have a look?

@alesj
Copy link
Contributor Author

alesj commented Oct 18, 2022 via email

@quarkus-bot

This comment has been minimized.

@quarkus-bot

This comment has been minimized.

@quarkus-bot

This comment has been minimized.

@quarkus-bot

This comment has been minimized.

@alesj alesj force-pushed the grpc2 branch 2 times, most recently from 3de6a30 to 5ea4b28 Compare October 25, 2022 13:32
@quarkus-bot

This comment has been minimized.

@quarkus-bot

This comment has been minimized.

Copy link
Member

@cescoffier cescoffier left a comment

Choose a reason for hiding this comment

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

I made a few comments, but it's really great!

shutdown.addShutdownTask(route::remove); // remove this route at shutdown, this should reset it
}

// TODO -- handle Avro, plain text ... when supported / needed
Copy link
Member

Choose a reason for hiding this comment

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

should we create an epic with the missing pieces (this one, stork...)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, that would be good -- so we don't miss out or forget.

integration-tests/pom.xml Outdated Show resolved Hide resolved
@cescoffier
Copy link
Member

We need to prepare:

  • documentation explaining how to use this mode and the rationale
  • the migration guide
  • a blog post explaining the concepts and when we will make this the default

@quarkus-bot

This comment has been minimized.

@geoand
Copy link
Contributor

geoand commented Oct 27, 2022

We need to prepare:

* documentation explaining how to use this mode and the rationale

* the migration guide

* a blog post explaining the concepts and when we will make this the default

Very much +1.

I've also added the noteworthy-feature label which can be well explained in the resources mentioned above

@quarkus-bot
Copy link

quarkus-bot bot commented Oct 28, 2022

Failing Jobs - Building 0adf6c1

Status Name Step Failures Logs Raw logs
✔️ Gradle Tests - JDK 11
Gradle Tests - JDK 11 Windows Build Failures Logs Raw logs

Full information is available in the Build summary check run.

Failures

⚙️ Gradle Tests - JDK 11 Windows #

- Failing: integration-tests/gradle 

📦 integration-tests/gradle

io.quarkus.gradle.BasicJavaLibraryModuleTest.testBasicMultiModuleBuild line 25 - More details - Source on GitHub

java.lang.AssertionError: 

Expecting path:

io.quarkus.gradle.devmode.BasicJavaLibraryModuleDevModeTest.main line 23 - More details - Source on GitHub

org.awaitility.core.ConditionTimeoutException: Condition with lambda expression in io.quarkus.test.devmode.util.DevModeTestUtils that uses java.util.function.Supplier, java.util.function.Supplierjava.util.concurrent.atomic.AtomicReference was not fulfilled within 3 minutes.
	at org.awaitility.core.ConditionAwaiter.await(ConditionAwaiter.java:167)
	at org.awaitility.core.CallableCondition.await(CallableCondition.java:78)

📦 integration-tests/gradle/target/classes/basic-java-library-module/application

org.acme.ApplicationConfigResourceTest.testAppConfigEndpoint() - More details - Source on GitHub

java.lang.RuntimeException: java.lang.RuntimeException: io.quarkus.builder.BuildException: Build failure: Build failed due to errors
	[error]: Build step io.quarkus.kubernetes.client.deployment.DevServicesKubernetesProcessor#setupKubernetesDevService threw an exception: java.lang.RuntimeException: java.lang.IllegalStateException: Could not find a valid Docker environment. Please see logs and check configuration
	at io.quarkus.kubernetes.client.deployment.DevServicesKubernetesProcessor.setupKubernetesDevService(DevServicesKubernetesProcessor.java:119)

@gsmet
Copy link
Member

gsmet commented Oct 31, 2022

@cescoffier I let you merge it if you think it's ready.

@cescoffier
Copy link
Member

Yes it is, I just wanted to target 2.15.

@gsmet
Copy link
Member

gsmet commented Oct 31, 2022

Well, then you're good to go as main is 2.15.

@cescoffier cescoffier merged commit 89a81fe into quarkusio:main Oct 31, 2022
@quarkus-bot quarkus-bot bot added this to the 2.15 - main milestone Oct 31, 2022
@rsvoboda
Copy link
Member

We need to prepare:

  • documentation explaining how to use this mode and the rationale
  • the migration guide
  • a blog post explaining the concepts and when we will make this the default

Atm there is

@alesj / @cescoffier / @gsmet

@cescoffier
Copy link
Member

@alesj ^

@alesj
Copy link
Contributor Author

alesj commented Jan 13, 2023

@rsvoboda this was added / commited:

The blog is wip ...
And I guess some more info at the migration guide would be useful?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants