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

Add grpc netty keep-alive-time configuration option #12415

Closed
wants to merge 4 commits into from
Closed

Add grpc netty keep-alive-time configuration option #12415

wants to merge 4 commits into from

Conversation

spike83
Copy link
Contributor

@spike83 spike83 commented Sep 30, 2020

While using gRPC server side stream we are facing stream termination due to short tcp idle time on loadbalancer. There is configuration options in netty server which are not exposed presently.
With this the netty keep-alive is exposed as quarkus.grpc.server.netty.keep-alive-timewhere we can set a Duration.

Copy link
Member

@gsmet gsmet left a comment

Choose a reason for hiding this comment

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

Thanks. Added a small comment but I'll let @michalszynkiewicz and @cescoffier handle this one.

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.

Can you add a test?

@spike83
Copy link
Contributor Author

spike83 commented Oct 5, 2020

Can you add a test?

@cescoffier Sure, what kind of test would you expect. You may point me to some similar tests?

@michalszynkiewicz
Copy link
Member

Can you add a test?

@cescoffier Sure, what kind of test would you expect. You may point me to some similar tests?

I can't speak for Clement but I think the following would be good to test:

  • the connection times out successfully after keep-alive time + epsilon
  • the connection doesn't time out if it doesn't

I think it would be okay to add the tests to extensions/grpc/deployment. You can find a few examples there already.

@cescoffier
Copy link
Member

@michalszynkiewicz what should we do with this PR?

@michalszynkiewicz
Copy link
Member

I just came back from PTO, I added creating a test for it to my todo list.
Do you think it's worth merging without a test?

@spike83
Copy link
Contributor Author

spike83 commented Jan 12, 2021

Unfortunately this went out of my focus. Sorry for that. I was looking into testing a while ago and had no clue how to nail it down. In the meantime we found that we need some gossip on higher level which keeps the connection open anyway. But it would be great if you could finish it. Thanks.

@cescoffier
Copy link
Member

We should rebase and see what's the CI says.

Base automatically changed from master to main March 12, 2021 15:55
@gsmet
Copy link
Member

gsmet commented Apr 1, 2021

This PR now conflicts with the main branch. I personally think it could be merged even without a test. But that's not my say.

@cescoffier @michalszynkiewicz what should we do with it?

@michalszynkiewicz
Copy link
Member

michalszynkiewicz commented Apr 1, 2021

@gsmet it should already be in: #14957
sorry, I forgot to close the original

@quarkus-bot quarkus-bot bot added the triage/invalid This doesn't seem right label Apr 1, 2021
@gsmet
Copy link
Member

gsmet commented Apr 1, 2021

Great, thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
triage/invalid This doesn't seem right
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants