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 MaxConnectionAge config. #5311

Merged
merged 2 commits into from
Mar 2, 2021
Merged

Add gRPC MaxConnectionAge config. #5311

merged 2 commits into from
Mar 2, 2021

Conversation

jsha
Copy link
Contributor

@jsha jsha commented Feb 26, 2021

This allows servers to tell clients to go away after some period of time, which triggers the clients to re-resolve DNS.

Per grpc/grpc#12295, this is the preferred way to do this.

Related: #5307.

This allows servers to tell clients to go away after some period of
time, which triggers the clients to re-resolve DNS.
@jsha jsha requested a review from a team as a code owner February 26, 2021 02:01
aarongable
aarongable previously approved these changes Feb 26, 2021
@andygabby andygabby self-requested a review March 1, 2021 22:32
@jsha
Copy link
Contributor Author

jsha commented Mar 1, 2021

Closing and reopening to try and trigger GitHub CI.

@jsha jsha closed this Mar 1, 2021
@jsha jsha reopened this Mar 1, 2021
cmd/config.go Outdated Show resolved Hide resolved
Copy link
Member

@beautifulentropy beautifulentropy left a comment

Choose a reason for hiding this comment

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

Godoc syntax

@andygabby
Copy link
Member

andygabby commented Mar 1, 2021

Adding an approval, because the change looks good, but a question to consider.

Do we want to combine this with setting the MaxConnectionAgeGrace flag too?

If I am reading it right, it appears to be an additive time after MaxConnectionAge where the connection will be forcibly closed. This would ensure that nothing could keep an open connection that hasn't read the new DNS record. It seems like something we might want to tune.

Here is a reference to tuning it if you are using the keepalive to periodically resolve DNS.

andygabby
andygabby previously approved these changes Mar 1, 2021
Co-authored-by: Samantha <hello@entropy.cat>
@jsha jsha dismissed stale reviews from andygabby and aarongable via 224cdfe March 1, 2021 23:27
@jsha
Copy link
Contributor Author

jsha commented Mar 1, 2021

I think we don't really need a sub-infinite grace period, because all of our requests have fairly short timeouts (sub-5m), and we trust the timeout mechanism. But if I turn out to be wrong, we can always add that later!

@jsha jsha merged commit b4e483d into main Mar 2, 2021
@jsha jsha deleted the max-connection-age branch March 2, 2021 02:37
beautifulentropy pushed a commit that referenced this pull request Mar 12, 2021
This allows servers to tell clients to go away after some period of time, which triggers the clients to re-resolve DNS.

Per grpc/grpc#12295, this is the preferred way to do this.

Related: #5307.
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

4 participants