Conversation
Ensures intermediary proxies/gateways (AWS NLB notably) don't silently close connections after a configurable inactivity timeout expires.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #728 +/- ##
==========================================
- Coverage 62.52% 60.88% -1.64%
==========================================
Files 56 58 +2
Lines 4139 4372 +233
==========================================
+ Hits 2588 2662 +74
- Misses 1425 1579 +154
- Partials 126 131 +5 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
mikeee
left a comment
There was a problem hiding this comment.
I'm not a fan of adding env vars endlessly to address config options as I'm partial to the functional options pattern but I do think that having both would provide the best flexibility. I propose adding options to NewClient().
I think it would be pertinent to include some documentation of this as well, at the moment I'd hazard a guess that our sparse unexported env var declarations aren't the best for surfacing this config option.
Also raising the question - do you think an incorrect value provided should cause a failure to create a new client? Instead I think defaulting to the keepalive package's 10s when provided an invalid value and logging an error would provide less friction.
tthophan
left a comment
There was a problem hiding this comment.
Decision: request_changes · Confidence: 95%
The PR enables gRPC keep-alive by default to prevent connection termination by intermediary proxies. The implementation correctly uses time.Duration for the keep-alive interval, but the time.Duration(keepAliveSeconds) cast is incorrect as keepAliveSeconds is an integer representing seconds, not nanoseconds.
| authTokenUnaryInterceptor(at), | ||
| authTokenStreamInterceptor(at), | ||
| grpc.WithKeepaliveParams( | ||
| keepalive.ClientParameters{ |
There was a problem hiding this comment.
Incorrect duration conversion. time.Duration(keepAliveSeconds) treats the integer as nanoseconds. Use time.Duration(keepAliveSeconds) * time.Second instead.
Description
Ensures intermediary proxies/gateways (AWS NLB notably) don't silently close connections after a configurable inactivity timeout expires.
Checklist
Please make sure you've completed the relevant tasks for this PR, out of the following list: