Periodically refresh a collector's list of assigned producers #5326
Conversation
This file contains 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
|
This is a part of #5284. It adds a periodic task that refreshes the list of producers that a collector is supposed to be polling. There was no Nexus-side API for this, actually, so it's a bit bigger than I expected! This task internally removes any that are not in the list and ensures any that are. That lowers to a potential update of the information about the producer in the case where the task polling it already exists. |
- Add an endpoint in Nexus's internal API for listing the assigned producers for a collector. - Spawn a task in the `oximeter-collector` which will periodically fetch the list; remove any producers not in that list; and ensure any that are. - Adds the time of this last refresh to the `oximeter-collector` server's API for fetching info about the collector - Remove old use of `reqwest` directly to register collector, opt-in to the generated Nexus client. - Adds some type conversions in the nexus client crate to simplify these new interfaces
bnaecker
force-pushed
the
refresh-oximeter-producer-list
branch
from
6e04763 to
b8c720a
Compare
jgallagher
approved these changes
Mar 25, 2024
|
Most things have been addressed in f44da6f. The only thing I'm still looking at is removing Nexus sending the list of producers to a collector when it registers. I think that'll be a pretty small change, but I want to add a way to start the refresh loop after we register, to reduce latency on the first startup. |
The oximeter collector already requests all its producers whenever it restarts (and periodically to stay up-to-date). Let that mechanism do the work, rather than making many small requests from Nexus to the collector when it registers.
|
@jgallagher Thanks for your eyes on this! I think I hit everything you brought up. Here is a snippet of the |
|
This test failure is real, and I think we can actually delete the test now. It's a regression for #4498, in which I fumbled the pagination and only sent the first page of producers to a newly-registered collector. That's not relevant any more since the collector fetches things from Nexus, not the other way around. |
|
Talked offline with @jgallagher, removing the outdated Nexus test and setting to automerge. |
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.
oximeter-collectorwhich will periodically fetch the list; remove any producers not in that list; and ensure any that are.oximeter-collectorserver's API for fetching info about the collectorreqwestdirectly to register collector, opt-in to the generated Nexus client.