Nexus: Add an inventory_loader background task (PR 1/2)
#9148
Conversation
| println!( | ||
| " loaded latest inventory collection as of {}: \ | ||
| collection {collection_id}, taken at {}", | ||
| humantime::format_rfc3339_millis(time_loaded.into()), | ||
| humantime::format_rfc3339_millis(time_started.into()), | ||
| ); |
There was a problem hiding this comment.
turbo nit: looking at the expectorate output, this ends up formatting a pretty long line, i might consider splitting it so it's less likely to wrap in smallish terminals?
maybe something like:
| println!( | |
| " loaded latest inventory collection as of {}: \ | |
| collection {collection_id}, taken at {}", | |
| humantime::format_rfc3339_millis(time_loaded.into()), | |
| humantime::format_rfc3339_millis(time_started.into()), | |
| ); | |
| println!( | |
| " loaded latest inventory collection as of {}:\n\ | |
| collection {collection_id}\n\ | |
| taken at {}", | |
| humantime::format_rfc3339_millis(time_loaded.into()), | |
| humantime::format_rfc3339_millis(time_started.into()), | |
| ); |
| return InventoryLoadStatus::Loaded { | ||
| collection_id: old_id, | ||
| time_started: old_time_started, | ||
| time_loaded, | ||
| }; |
There was a problem hiding this comment.
nitpicky: it could be worth including something in the status message that explicitly indicates whether the collection was newly loaded on this activation or not?
There was a problem hiding this comment.
I originally had it that way, but I changed it for a maybe-bad reason: I think it makes the omdb expectorate test flaky (in that we have a race between omdb seeing the first activation that loaded a collection vs seeing a subsequent activation that didn't).
There was a problem hiding this comment.
ah, yeah, that's fair. perhaps it could be redacted, but that might be more work than it's worth.
|
|
||
| pub struct InventoryLoader { | ||
| datastore: Arc<DataStore>, | ||
| tx: watch::Sender<Option<Arc<Collection>>>, |
There was a problem hiding this comment.
note that a watch::Receiver is basically just "an Arc plus some other stuff", so depending on usage patterns, it may not be necessary to have two layers of reference counting. however, this does let us hold onto prior inventories if they're still being used when a new one is loaded, so this may be desirable...
There was a problem hiding this comment.
however, this does let us hold onto prior inventories if they're still being used when a new one is loaded, so this may be desirable...
Yeah, this is exactly what I want - a Collection is pretty big, so cloning it is far from free, and I don't want the consumers of this channel to keep it locked while they act on it. This more visible on #9149; e.g.,
// Get the inventory most recently seen by the inventory loader
// background task. We clone the Arc to avoid keeping the channel locked
// for the rest of our execution.
let Some(collection) =
self.rx_inventory.borrow_and_update().as_ref().map(Arc::clone)
else {
// ...
};There was a problem hiding this comment.
yup, makes sense! perhaps worth commenting?
…PR 2/2) (#9149) (This is the second half of #9148.) As of this PR, all (I believe!) the background tasks that want to act on the latest inventory collection now act on the latest inventory collection loaded by the `inventory_loader` bg task. That task runs frequently. Fixes #8882. It's still possible two Nexuses could duel if one is acting on a newer inventory collection, but that's always true. The window for that should now be ~15 seconds, instead of waiting until the next time _our own Nexus_ collects inventory. I did not change other places in Nexus where we're reading inventory outside of background tasks. I think it would probably make sense to audit for those and make them read from this watch channel too? If that sounds right I can file an issue.
This adds a background task that periodically attempts to read the latest inventory collection from the DB. It first does a query for just the latest collection ID, then only does the full set of queries to load the entire collection if the ID has changed from the last one it read. Therefore, I set the period pretty aggressively (15 seconds), which is what #5296 suggested. (Neither this PR nor the followup do the rest of #5296; i.e., changing how the
inventory_collectortask works.)As of this PR, we only introduce the task and use it in tests. The followup PR changes all the other background tasks that currently read collections from the DB to read from this task's watch channel instead.