[Refactor]: Remove the use of channels to signal healthcheck status #5307
Labels
Comments
4 tasks
WillSewell
pushed a commit
to WillSewell/jaeger
that referenced
this issue
Mar 28, 2024
Simplifies the signalling of healthcheck status from the "servers" to the "service": instead of using 2 channels to feed healthcheck status back to the Service.HealthCheck, we just give the server components direct access to the Healthcheck which they can update directly. This is possible because the Healthcheck package is threadsafe (uses `atomic.Value` for state). This pattern is consisten with how the service's Healtcheck is passed directly to cmd/collector/app package. closes jaegertracing#5307 Signed-off-by: Will Sewell <[email protected]>
4 tasks
|
I took the "pass directly" approach here: #5308. Interested to get your thoughts @yurishkuro as it's a bit different to the callback approach you mentioned. |
|
passing directly is ok. The alternative would be to just pass the function |
yurishkuro
added a commit
that referenced
this issue
Mar 28, 2024
## Which problem is this PR solving? Resolves #5307 ## Description of the changes - Simplifies the signalling of healthcheck status from the "servers" to the "service": instead of using 2 channels to feed healthcheck status back to the Service.HealthCheck, we just give the server components direct access to the Healthcheck which they can update directly. - This is possible because the Healthcheck package is threadsafe (uses `atomic.Value` for state). - This pattern is consistent with how the service's Healtcheck is passed directly to cmd/collector/app package. ## How was this change tested? - `make lint test` ## Checklist - [x] I have read https://github.com/jaegertracing/jaeger/blob/master/CONTRIBUTING_GUIDELINES.md - [x] I have signed all commits - [ ] I have added unit tests for the new functionality - [x] I have run lint and test steps successfully - for `jaeger`: `make lint test` - for `jaeger-ui`: `yarn lint` and `yarn test` --------- Signed-off-by: Will Sewell <[email protected]> Signed-off-by: Yuri Shkuro <[email protected]> Co-authored-by: Yuri Shkuro <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Requirement
As a contributor who has been attempting to fix goroutine leaks in tests it would be easier to manage gorotoutine cleanup on server close if healthcheck reporting between the server and service did not rely on channels.
Problem
As discussed in https://github.com/jaegertracing/jaeger/pull/5299/files#diff-b4a37944a109260d3630b973cdae2dc012d6bc67134efa50397976f713630e08, a significant source of complexity and risk of deadlocks comes from the use of a channel to signal server healthcheck status back to the server.
Proposal
Instead of using a channel,
pkg/healthcheckis already thread safe, and we currently don't have any need for components to "subscribe" to healthcheck status, so instead of writing to a channel and having a consumer update thepkg/healthcheck, I think we could just update the state from the server directly.Open questions
I'm not sure whether we should completely decouple the server from the healthcheck package, by passing in a callback to update healthcheck status. The alternative would be to just pass a
*healthcheck.Healthcheckdirectly to the server.Pros of callback: gives the caller more control - e.g. they could use a channel if they wanted to
Cons: inconsistent with how the
*healthcheck.Healthcheckis passed directly to the collector component (example), I also think it's a bit more complex due to indirection.Currently I'm leaning towards passing a
*healthcheck.Healthcheckbecause there's currently no need for anything more flexible - e.g. the ability to subscribe to changes - and I think if this ever was required it's something that could be built into the healthcheck package.The text was updated successfully, but these errors were encountered: