Reduce oximeter's reliance on async channels
#8663
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
- Use `watch` channels to communicate between `oximeter`'s tasks, instead of `mpsc`. This reduces async and avoids questions of how to handle full queues. - Use a sync `Mutex` instead of an async one internally, since creating a collection task and most operations on it are synchronous (through the watch channels). - This is all intended to help with the responsiveness issues around #8595. In that case, a slow task inserting data into the database can wedge `oximeter` entirely, since the queues used internally have filled up. By moving to `watch` channels, we can at least ask questions about `oximeter`'s state even if the tasks actually doing collections or database insertions are stuck or slow.
smklein
reviewed
Jul 22, 2025
| @@ -1017,29 +991,6 @@ mod tests { | |||
| logctx.cleanup_successful(); | |||
| } | |||
|
|
|||
| #[tokio::test] | |||
| async fn test_delete_nonexistent_producer_succeeds() { | |||
There was a problem hiding this comment.
Does this test no longer work with the new APIs?
(I know there's no return code, but might not be bad to assert that it doesn't panic)
There was a problem hiding this comment.
It does work, but I thought it was not very useful since the method is now infallible. I can put it back, I don't feel very strongly either way.
|
I ran an ad-hoc test locally, trying to repro similar behavior to what we see in #8595 on Dogfood. I installed Omicron locally on my dev machine, and manually ran and During this time, we can continue to query oximeter for its state, including the list of producers and the details from those: Eventually, those errors become visible in the producer details: Restarting the ClickHouse service with So |
jgallagher
reviewed
Jul 23, 2025
- Don't store producer endpoint info - Use watch instead of notify for shutdown - Consume self in shutdown method
jgallagher
approved these changes
Jul 23, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
watchchannels to communicate betweenoximeter's tasks, instead ofmpsc. This reduces async and avoids questions of how to handle full queues.Mutexinstead of an async one internally, since creating a collection task and most operations on it are synchronous (through the watch channels).oximeterentirely, since the queues used internally have filled up. By moving towatchchannels, we can at least ask questions aboutoximeter's state even if the tasks actually doing collections or database insertions are stuck or slow.