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

Grpc netty client improvements #46198

Merged
merged 1 commit into from
Mar 10, 2025
Merged

Conversation

franz1981
Copy link
Contributor

@franz1981 franz1981 commented Feb 11, 2025

Fixes #46148

Copy link

quarkus-bot bot commented Feb 11, 2025

Thanks for your pull request!

Your pull request does not follow our editorial rules. Could you have a look?

  • description should not be empty, describe your intent or provide links to the issues this PR is fixing (using Fixes #NNNNN) or changelogs

This message is automatically generated by a bot.

@franz1981 franz1981 force-pushed the grpc_netty_client branch 2 times, most recently from 6ae867f to 6170715 Compare February 11, 2025 14:55
@franz1981 franz1981 marked this pull request as ready for review February 11, 2025 17:41

This comment has been minimized.

@franz1981
Copy link
Contributor Author

@alesj I think I broke internet here eheh

@franz1981
Copy link
Contributor Author

I will check the failures tomorrow but I see @cescoffier told in the related issue

When using gRPC Java, we definitely need to avoid reusing the same event loops (we could add this as an option)

Grpc Netty should be safe (in theory?) - or you meant the grpc Java netty provider too?

@franz1981
Copy link
Contributor Author

franz1981 commented Feb 11, 2025

Interesting: see

java.lang.IllegalStateException: Both EventLoopGroup and ChannelType should be provided or neither should be

Which means that we have to know how vertx transport is configured and reuse it to configure the grpc Netty channel (nio/epoll/ whatever) in order to work, which kind of make Sense since Netty will complain the same If the event loop used isn't of the "right type" - a limit which have lifted on Netty 4.2 (partially)

@franz1981 franz1981 force-pushed the grpc_netty_client branch 2 times, most recently from 2d06e28 to 8fdb4b8 Compare February 12, 2025 10:32
@quarkus-bot quarkus-bot bot added the area/dependencies Pull requests that update a dependency file label Feb 12, 2025

This comment has been minimized.

@franz1981
Copy link
Contributor Author

that looks better @alesj wdyt?

This comment has been minimized.

@franz1981
Copy link
Contributor Author

@cescoffier @geoand do we want a special config to enable this?

@geoand
Copy link
Contributor

geoand commented Feb 13, 2025

Probably makes sense

@cescoffier
Copy link
Member

Good question. I would say yes and enable it by default. Someone can roll back to the previous behavior if something goes wrong.

@franz1981
Copy link
Contributor Author

@cescoffier I'll make it a proper runtime config? Documented? wdyt? 🙏

@cescoffier
Copy link
Member

@franz1981 Yes, runtime config would be great!

@franz1981
Copy link
Contributor Author

franz1981 commented Feb 19, 2025

need some help @cescoffier and @alesj 👍
I see that we create the grpc producer (and the new code I've built) at build time via

ResultHandle result = mc.invokeStaticMethod(CREATE_CHANNEL_METHOD, name, interceptorsSet);
but it won't pass runtime properties: how i can pass them?
The other problem is that I keep on reading the sysprop re reusing buffers with it, while I probably just read it once.

@alesj
Copy link
Contributor

alesj commented Feb 19, 2025 via email

@alesj
Copy link
Contributor

alesj commented Mar 3, 2025

You can pass (your modified) GrpcClientBuildTimeConfig to generateGrpcClientProducers ... and then to generateChannelProducer ...

@franz1981
Copy link
Contributor Author

@alesj @cescoffier I've added the option to use a runtime config instead, to keep it more flexible - since the loaded classes will basically the same

@franz1981 franz1981 force-pushed the grpc_netty_client branch from 48b6c1d to b32fba7 Compare March 3, 2025 10:38

This comment has been minimized.

Copy link

github-actions bot commented Mar 3, 2025

🎊 PR Preview 58ff745 has been successfully built and deployed to https://quarkus-pr-main-46198-preview.surge.sh/version/main/guides/

  • Images of blog posts older than 3 months are not available.
  • Newsletters older than 3 months are not available.

This comment has been minimized.

@franz1981
Copy link
Contributor Author

ready to go @cescoffier ? wdyt?

@cescoffier cescoffier added the triage/waiting-for-ci Ready to merge when CI successfully finishes label Mar 10, 2025
Copy link

quarkus-bot bot commented Mar 10, 2025

Status for workflow Quarkus Documentation CI

This is the status report for running Quarkus Documentation CI on commit bf12ff2.

✅ 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.

Warning

There are other workflow runs running, you probably need to wait for their status before merging.

Copy link

quarkus-bot bot commented Mar 10, 2025

Status for workflow Quarkus CI

This is the status report for running Quarkus CI on commit bf12ff2.

✅ 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.

@geoand geoand merged commit d3a8d95 into quarkusio:main Mar 10, 2025
45 checks passed
@quarkus-bot quarkus-bot bot removed the triage/waiting-for-ci Ready to merge when CI successfully finishes label Mar 10, 2025
@quarkus-bot quarkus-bot bot added this to the 3.21 - main milestone Mar 10, 2025
@quarkus-bot quarkus-bot bot added the kind/enhancement New feature or request label Mar 10, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/dependencies Pull requests that update a dependency file area/grpc gRPC area/vertx kind/enhancement New feature or request triage/flaky-test
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Grpc's event loops are kept separated from the Rest ones
4 participants