doc: update keepalive ClientParameters doc about doubling the interval upon GOAWAY #7469
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #7469 +/- ##
=======================================
Coverage 81.38% 81.38%
=======================================
Files 354 354
Lines 27080 27080
=======================================
Hits 22040 22040
Misses 3831 3831
Partials 1209 1209 |
There was a problem hiding this comment.
While you are here, should we also try and fix the code where we check if Time is less than 10s and if so, set it to 10s. We should check for 0s explicitly there and probably not set it to 10s if the value is 0s? This will be a behavior change though. @dfawley
There was a problem hiding this comment.
I'm undecided on the 0s vs. 10s. vs infinity question so far.
The question to me is: if the user actually sets keepalive, why would they want it to have an infinite Time? Is there ever a reason to do that? If not, then the default being 10s (when configuring keepalive) seems pretty reasonable to me.
If we keep the behavior the same, we should update the comment, though, to say "keepalives are disabled by default, but the default value for this field is 10s".
keepalive/keepalive.go
Outdated
| // disconnects due to its enforcement policy. | ||
| // | ||
| // For more details, see | ||
| // https://github.com/grpc/proposal/blob/master/A8-client-side-keepalive.md | ||
| Time time.Duration // The current default value is infinity. |
There was a problem hiding this comment.
Can you delete this comment, please? Explaining the behavior can be done via the existing text above and in grpc.WithKeepaliveParams (which ideally would be updated as part of this cleanup, if you don't mind, to say something like "Keepalive is disabled by default" or "Keepalive pings are disabled by default").
keepalive/keepalive.go
Outdated
| @@ -33,7 +33,19 @@ import ( | |||
| type ClientParameters struct { | |||
| // After a duration of this time if the client doesn't see any activity it | |||
| // pings the server to see if the transport is still alive. | |||
| // If set below 10s, a minimum value of 10s will be used instead. | |||
| // If set below 10s (including 0), a minimum value of 10s will be used | |||
There was a problem hiding this comment.
The "including 0" is probably not needed if we get rid of the comment below as mentioned.
Add to the godoc the behavior that the client doubles the keepalive ping interval when GOAWAY is received due to too-many-pings.
And link to the design doc.
RELEASE NOTES: N/A