Implementation for server enforcement of keepalive policy. #1147
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
menghanl
reviewed
Mar 27, 2017
| } | ||
| case <-timeout.C: | ||
| t.Fatalf("Test failed: Expected a GoAway from server.") | ||
| } |
There was a problem hiding this comment.
Also check for connection closed.
dfawley
reviewed
Mar 29, 2017
transport/http2_server.go
Outdated
| if ns < 1 && !t.kep.PermitWithoutStream { | ||
| // Keepalive shouldn't be active thus, this new ping should | ||
| // have come after atleast two hours. | ||
| if t.lastPingAt.Add(2 * time.Hour).After(now) { |
There was a problem hiding this comment.
This 2*time.Hour should probably be a constant (next to maxPingStrikes?).
transport/transport_test.go
Outdated
| } | ||
| clientOptions := ConnectOptions{ | ||
| KeepaliveParams: keepalive.ClientParameters{ | ||
| Time: 100 * time.Millisecond, |
There was a problem hiding this comment.
Should this be 101ms (or more) to avoid a race between the timers?
transport/transport_test.go
Outdated
| } | ||
| clientOptions := ConnectOptions{ | ||
| KeepaliveParams: keepalive.ClientParameters{ | ||
| Time: 100 * time.Millisecond, |
transport/transport_test.go
Outdated
|
|
||
| // Give keepalive enough time. | ||
| time.Sleep(2 * time.Second) | ||
| // Asser that connection is healthy |
transport/transport_test.go
Outdated
|
|
||
| // Give keepalive enough time. | ||
| time.Sleep(2 * time.Second) | ||
| // Asser that connection is healthy |
keepalive/keepalive.go
Outdated
|
|
||
| // EnforcementPolicy is used to set keepalive enforcement policy on the server-side. | ||
| // Server will close connection with a client that violates this policy. | ||
| type EnforcementPolicy struct { |
There was a problem hiding this comment.
Should we have some notes in the ClientParameters to the effect of "make sure you set these parameters in coordination with the service owners, as incompatible settings can result in RPC failures"?
Especially since there's a default enforcement policy of 5m and no pings without streams, even if the service doesn't declare a policy.
keepalive/keepalive.go
Outdated
| // Server will close connection with a client that violates this policy. | ||
| type EnforcementPolicy struct { | ||
| // MinTime is the minimum amount of time a client should wait before sending a keepalive ping. | ||
| MinTime time.Duration |
There was a problem hiding this comment.
Would you mind documenting the default values for all of the settings in this file?
dfawley
approved these changes
Mar 31, 2017
bdarnell
added a commit
to bdarnell/cockroach
that referenced
this pull request
Aug 16, 2017
This error code is used for fail-fast errors (which can be retried unambiguously), but it is also used in other cases (such as a server draining) in which we cannot assume that the previous attempt was not completed. (It's unclear whether this assumption was once true and changed or if it's always been incorrect. The specific source of ambiguous Unavailable errors we're seeing is grpc/grpc-go#1147) This is expected to increase prevalence of AmbiguousResultErrors; this will be fixed in a follow-up change. Fixes cockroachdb#17491
bdarnell
added a commit
to bdarnell/cockroach
that referenced
this pull request
Aug 16, 2017
This error code is used for fail-fast errors (which can be retried unambiguously), but it is also used in other cases (such as a server draining) in which we cannot assume that the previous attempt was not completed. (It's unclear whether this assumption was once true and changed or if it's always been incorrect. The specific source of ambiguous Unavailable errors we're seeing is grpc/grpc-go#1147) This is expected to increase prevalence of AmbiguousResultErrors; this will be fixed in a follow-up change. Fixes cockroachdb#17491
bdarnell
added a commit
to bdarnell/cockroach
that referenced
this pull request
Aug 17, 2017
This error code is used for fail-fast errors (which can be retried unambiguously), but it is also used in other cases (such as a server draining) in which we cannot assume that the previous attempt was not completed. (It's unclear whether this assumption was once true and changed or if it's always been incorrect. The specific source of ambiguous Unavailable errors we're seeing is grpc/grpc-go#1147) This is expected to increase prevalence of AmbiguousResultErrors; this will be fixed in a follow-up change. Fixes cockroachdb#17491
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
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.
No description provided.