Make gRPC ServerParameters configurable #499
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
kentquirk
reviewed
Sep 4, 2022
|
|
||
| # After a duration of this time if the server doesn't see any activity it | ||
| # pings the client to see if the transport is still alive. | ||
| # If set below 1s, a minimum value of 1s will be used instead. |
There was a problem hiding this comment.
According to the docs here, the lower limit is 10s, not 1s, unless I'm reading the wrong thing. Can you check please, and also note the meaning of 0s?
There was a problem hiding this comment.
Thanks for pointing that out @kentquirk! Although, it looks like what you linked to is for the ClientParameters.
I grabbed the comments here for the ServerParameters.
I've also pushed a commit for adding notes about the meaning of 0s!
MikeGoldsmith
added
type: enhancement
MikeGoldsmith
requested changes
Sep 6, 2022
There was a problem hiding this comment.
As per @kentquirk's request. Please check min value and update docs 👍🏻
MikeGoldsmith
added
the
status: revision needed
abatilo
requested review from
MikeGoldsmith and
kentquirk
and removed request for
emilyashley,
MikeGoldsmith and
kentquirk
|
I think there might be a bug with GitHub. I tried to re-request reviews from both @kentquirk and @MikeGoldsmith but it seems to have removed @emilyashley and then says I need to re-request review from either Kent or Mike. And if I re-request one, the other gets shown as needing a re-request.
Whatever GitHub did, I updated the docs and commented about the min value in question! |
kentquirk
approved these changes
Sep 6, 2022
kentquirk
added
status: revision needed
added 2 commits
September 6, 2022 17:20
Which problem is this PR solving -------------------------------- At the moment, there is no way to override the ServerParameters used in the `keepalive` configuration of the gRPC server. As to @bdarfler's comments in honeycombio#263, issues can arise where new hosts will never be connected to because clients will reconnect to already existing hosts and by the time replacement hosts are available, all clients will have reconnected to existing hosts with a `MaxConnectionAge`/`MaxConnectionAgeGrace` of [infinity](https://github.com/grpc/grpc-go/blob/60a3a7e969c401ca16dbcd0108ad544fb35aa61c/internal/transport/defaults.go#L36-L37). Short description of the changes -------------------------------- The changes made are to plumb through the configuration of the GRPC ServerParameters from the viper configuration and into the instantiation of the actual gRPC server. These changes kept the previous default configuration of refinery as the defaults of the configurable settings, so anyone who doesn't explicitly override any of these configurations should have exactly the same behavior. Closes honeycombio#263
abatilo
force-pushed
the
aaron.grpc-server-parameters
branch
3 times, most recently
from
afe5d56 to
d9d00d7
Compare
Let's explicitly explain what 0 values mean for every field.
abatilo
force-pushed
the
aaron.grpc-server-parameters
branch
from
d9d00d7 to
7b400f5
Compare
MikeGoldsmith
approved these changes
Sep 7, 2022
ghost
pushed a commit
to opsramp/tracing-proxy
that referenced
this pull request
Jul 5, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Which problem is this PR solving
At the moment, there is no way to override the ServerParameters used in
the
keepaliveconfiguration of the gRPC server. As to @bdarfler'scomments in #263, issues can arise where new hosts will never be
connected to because clients will reconnect to already existing hosts
and by the time replacement hosts are available, all clients will have
reconnected to existing hosts with a
MaxConnectionAge/MaxConnectionAgeGraceofinfinity.
Short description of the changes
The changes made are to plumb through the configuration of the GRPC
ServerParameters from the viper configuration and into the instantiation
of the actual gRPC server. These changes kept the previous default
configuration of refinery as the defaults of the configurable settings,
so anyone who doesn't explicitly override any of these configurations
should have exactly the same behavior.
Closes #263