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

Fix/grpc keepalive #903

Merged
merged 8 commits into from
Jun 27, 2023
Merged

Fix/grpc keepalive #903

merged 8 commits into from
Jun 27, 2023

Conversation

Despire
Copy link
Contributor

@Despire Despire commented Jun 26, 2023

closes #885

by setting the keepalive params for a client connection and adding a retry policy on RPC that fail due to code unavailable

Unavailable indicates the service is currently unavailable

The client will try for roughly ~10 minutes for the service to become available and retry the RPC, given that our calls are idempotent this shouldn't be an issues.

It may happen that two calls at the same time will be executed (the one previosly started where the connection was lost and the new one), however. In terraformer we use a lock so that shouldn't be an issues and with the rest of the calls it shouldn't raise any negative side effects.

internal/utils/grpc.go Outdated Show resolved Hide resolved
internal/utils/grpc.go Outdated Show resolved Hide resolved
internal/utils/grpc.go Outdated Show resolved Hide resolved
Copy link
Contributor

@MiroslavRepka MiroslavRepka left a comment

Choose a reason for hiding this comment

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

LGTM, thanks @Despire!

Copy link
Contributor

@cloudziu cloudziu left a comment

Choose a reason for hiding this comment

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

Awesome! Thanks :D

@Despire
Copy link
Contributor Author

Despire commented Jun 26, 2023

Had to also adjust the server settings as otherwise the server would just sent

[transport] Client received GoAway with error code ENHANCE_YOUR_CALM and debug data equal to ASCII "too_many_pings".

Most values are the same as the defaults, which can be found https://grpc.io/docs/guides/keepalive/#keepalive-configuration-specification

The only changes are in keepalive.EnforcementPolicy.MinTime which is set to 45 seconds to reflect the same time as in the client.

and keepalive.ServerParameters.Timeout is set to 5 minutes same as with the client

@Despire Despire added this pull request to the merge queue Jun 27, 2023
Merged via the queue into master with commit 6815056 Jun 27, 2023
1 check was pending
@Despire Despire deleted the fix/grpc-keepalive branch June 27, 2023 06:02
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.

Feature: Make gRPC connections more robust
3 participants