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

Expose grpc keepalive params in config #1094

Merged
merged 5 commits into from
Jun 21, 2022
Merged

Conversation

d80tb7
Copy link
Collaborator

@d80tb7 d80tb7 commented Jun 18, 2022

Expose GRPC keepalive parameters via Armada configuration (definitions can be found here). The motivation here is to allow users to tweak these parameters based on their particular Armada deployment (e.g. to increase the ping frequency for quicker notification of transport failures at the expense of server load).

  • ServerParameters.MaxConnectionIdle
  • ServerParameters.MaxConnectionAge
  • ServerParameters.MaxConnectionAgeGrace
  • ServerParameters.Time
  • ServerParameters.Timeout
  • EnforcementPolicy.MinTime
  • EnforcementPolicy.PermitWithoutStream

I've added default values to config/armada/config.yaml, config/lookout/config.yaml and config/binoculars/config.yaml[1] so that all values will be defaulted to the values that were set in previous releases.

[1] I've omitted ServerParameters.MaxConnectionAge and ServerParameters.MaxConnectionAgeGrace from the default values as these would need to be set to either int64.maxValue or 0s to maintain the current behaviour, which I think is a bit cryptic. Instead by leaving them unset they will default to 0, which the grpc lib will treat as unset and apply the default value of infinity.

Copy link
Contributor

@kannon92 kannon92 left a comment

Choose a reason for hiding this comment

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

Looks good to me! Thanks for adding this.

Have you looked at how these options match to clients? I didn't see a direct 1to1 from go to python. It may be fun trying to match options with the different clients.

@d80tb7
Copy link
Collaborator Author

d80tb7 commented Jun 21, 2022

@kannon92

So I think the main one we care about here from the client's point of view is EnforcementPolicy.MinTime. Basically, if a client tries to heartbeat more frequently than this the server will start rejecting it. I think the relevant python client channel setting is grpc.keepalive_time_ms.

@d80tb7 d80tb7 enabled auto-merge (squash) June 21, 2022 09:16
@d80tb7 d80tb7 merged commit 237b672 into master Jun 21, 2022
@jimbobby5 jimbobby5 deleted the f/chrisma/grpc-heartbeats branch October 26, 2022 02: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.

None yet

3 participants