Add periodic health checks to TChannel #318
Conversation
connection.go
Outdated
| } | ||
| if timeout == 0 { | ||
| timeout = time.Second | ||
| } |
There was a problem hiding this comment.
Do we want to validate/enforce that timeout <= interval?
|
The "health" branch of tchannel-go is good for ringpop: if destination blackholes, we see that within 1 second, without filling any outgoing buffers, and ringpop has a better chance to detect the transport error. I am all-in for this feature. |
|
Is there any concerns about having constant pings on any TCP connection? tchannel-go does not currently close working TCP connections, so over time, if ringpop ends up connecting to every other host, each of those connections will have constant health checks. The main concern I have about this change is the default health timeout period, since some services may have much slower responses. We could even ship this change but have a pretty high value (like 10 seconds or something) -- although I imagine ringpop would want a much lower value. Since it doesn't create the channel, the service owner would need to set a lower value, which seems like extra configuration that we could avoid by having a lower default. |
connection.go
Outdated
| } | ||
| return | ||
| } | ||
| cancel() |
There was a problem hiding this comment.
is ok that cancel() is not called when this returns in L948?
|
@prashantv is there an eta for this change? |
|
I'll probably not get to this in the next week, there's a bunch of relay work that's higher priority right now. |
prashantv
force-pushed
the
health
branch
3 times, most recently
from
12ab0d6
to
5d979b3
Compare
|
@prashantv Are there any plans to get this in? Looks like it's been over a year since it was originally created. This would be useful for cadence. |
prashantv
changed the title
prashantv
force-pushed
the
health
branch
3 times, most recently
from
7e8859d
to
7ce4c7e
Compare
Codecov Report
@@ Coverage Diff @@
## dev #318 +/- ##
==========================================
- Coverage 85.94% 85.51% -0.44%
==========================================
Files 37 38 +1
Lines 4668 4762 +94
==========================================
+ Hits 4012 4072 +60
- Misses 534 565 +31
- Partials 122 125 +3
Continue to review full report at Codecov.
|
|
I've updated the diff with full test coverage. Health checking is opt-in, and by default no active health checks are performed. The caller can choose the interval between checks, what the timeouts are for each ping message, and how many consecutive failures should cause a connection to be closed. |
If a TChannel connection stalls (e.g., the remote side has disappeared, and traffic starts black-holing), TChannel waits for TCP to detect this issue, but this is often not configurable, and it can be a long period of time (we've observed many minutes in production). In many P2P situations, it's desirable to detect these failures as soon as possible. Support active health checking using TChannel pings on a periodic basis, with configurable timeouts, number of failures etc.
health.go
Outdated
| } | ||
|
|
||
| ctx, cancel := context.WithTimeout(c.healthCheckCtx, opts.Timeout) | ||
| defer cancel() |
There was a problem hiding this comment.
Do we want to defer this until the function returns, or should we make sure to execute it at the end of this loop iteration?
There was a problem hiding this comment.
Good catch! Could have been a huge memory leak, changed ti to cancel immediately after the ping method, since we don't use the ctx for anything else.
|
I am very excited about this. |
If a TChannel connection stalls (e.g., the remote side has disappeared,
and traffic starts black-holing), TChannel waits for TCP to detect this
issue, but this is often not configurable, and it can be a long period
of time (we've observed many minutes in production).
In many P2P situations, it's desirable to detect these failures as soon
as possible. Support active health checking using TChannel pings on a
periodic basis, with configurable timeouts, number of failures etc.