Conversation
kakkoyun
force-pushed
the
simplify_probe
branch
2 times, most recently
from
750afc7 to
2aee205
Compare
|
Fixed the issues, now we only set |
|
ping @FUSAKLA |
FUSAKLA
left a comment
FUSAKLA
left a comment
There was a problem hiding this comment.
Great! 🎉
Sorry for the lag
I like it, makes it simpler. Just few questions about the changes of setting the ready status because it's not 100% clear to me how the run package handles the errors.
Otherwise 👍
|
|
||
| g.Add(func() error { | ||
| statusProber.SetReady() | ||
| statusProber.Ready() |
There was a problem hiding this comment.
It's feels bit confusing to set ready on gRPC start but not ready on HTTP stop 🤔
What is the motivation for this? Sorry I'm maybe missing something out :/
There was a problem hiding this comment.
Each component actually has a different life-cycle, as you already know, they set their ready state with a different flow. Some of the components have more complicated logic, e.g receive.
For the rest, just to set a convention I decided to set ready just before the component ready to serve gRPC traffic if they serve gRPC because http starts to serve slightly faster than gRPC and usually there are other steps before gRPC server initialized. Otherwise, I stick to the http being served since all of them serve HTTP.
There was a problem hiding this comment.
Furthermore, questions concerning oklog/run package, AFAIK whenever an error happens in a rungroup all of the error handlers get called sequentially with registration order, and under normal circumstances, this is a very fast process. However, we recently introduced the graceful shutdown of gRPC and HTTP servers. With those changes they can wait for connections to drain, up to 2m (which is the default query timeout). Since we always register the HTTP server first, I decided to set the probe to not ready, when HTTP server gets shut down, and stop receiving traffic.
There was a problem hiding this comment.
Thanks for clearing this up! LGTM 👍
There was a problem hiding this comment.
Plus it's worth to note that if HTTP server is drained, we are not ready anyway.
| component: component, | ||
| logger: logger, | ||
| status: prometheus.NewGaugeVec(prometheus.GaugeOpts{ | ||
| Name: "status", |
There was a problem hiding this comment.
The metric name change should be probably mentioned in changelog?
There was a problem hiding this comment.
👍 I'll update to change log.
Signed-off-by: Kemal Akkoyun <kakkoyun@gmail.com>
Signed-off-by: Kemal Akkoyun <kakkoyun@gmail.com>
Signed-off-by: Kemal Akkoyun <kakkoyun@gmail.com>
kakkoyun
force-pushed
the
simplify_probe
branch
from
69e3074 to
b62edc1
Compare
|
|
||
| g.Add(func() error { | ||
| statusProber.SetReady() | ||
| statusProber.Ready() |
There was a problem hiding this comment.
Plus it's worth to note that if HTTP server is drained, we are not ready anyway.
3 participants
This PR aims to simplify
proberimplementation by keeping its existing behaviour.It also tries to simplify API and tries to use more idiomatic go naming for methods.
It also moves prober API touchpoints to
server/httpcall sites to make the usage more explicit.These changes are purely cosmetic and just to increase the maintainability of the package, you can ignore this PR, if you think the changes don't provide any value.
Signed-off-by: Kemal Akkoyun kakkoyun@gmail.com
Changes
proberand its usages for sake of simplicity.Verification
Runs local test to ensure nothing changed,
make test-local.