Skip to content
This repository was archived by the owner on Mar 5, 2024. It is now read-only.

Issue #217 - Add GRPC keepalive to server/client#333

Closed
mehstg wants to merge 3 commits intouswitch:masterfrom
mehstg:grpc-keepalive-testing
Closed

Issue #217 - Add GRPC keepalive to server/client#333
mehstg wants to merge 3 commits intouswitch:masterfrom
mehstg:grpc-keepalive-testing

Conversation

@mehstg
Copy link
Copy Markdown

@mehstg mehstg commented Nov 25, 2019

This PR adds a keepalive between servers/clients ensuring connections are closed correctly in the case of ungraceful KIAM server termination.

For reference:
Kiam issue - #217
Go-GRPC issue: grpc/grpc-go#3206

Putting this forward for review. I am currently running this patch in our main development cluster with no ill effects. I am not a golang dev by any stretch of the imagination so am not sure of the knock on effects of enabling this.

@Joseph-Irving
Copy link
Copy Markdown
Contributor

Hi thanks for this contribution, it would be nice if these grpc options could be configured by flags, having sane defaults is always good but if people need to tweak or turn off this functionality it would be good to have the option.

RetryInterval = 10 * time.Millisecond
)

var kacp = keepalive.ClientParameters{
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

nit: I generally prefer to have variable names that are long and obvious instead of short and obscure, so keepAliveClientParams would be preferable over kacp.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Have updated the vars

@mehstg
Copy link
Copy Markdown
Author

mehstg commented Nov 27, 2019

Hi thanks for this contribution, it would be nice if these grpc options could be configured by flags, having sane defaults is always good but if people need to tweak or turn off this functionality it would be good to have the option.

I'm afraid this is probably a little beyond my limited golang skills. Happy for someone else to pick it up though.

@dbyron0
Copy link
Copy Markdown

dbyron0 commented Dec 4, 2019

#337 is enough to replace this?

@Joseph-Irving
Copy link
Copy Markdown
Contributor

thanks for trying this @mehstg!
this PR has implemented the same thing #337

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants