fix: reduce the work done while holding the updater write lock #504
Conversation
| ) { | ||
| // Do the check before matching so that the read lock can be released right away. | ||
| let result = filter.read().await.fetch_new_settings(storage_client).await; | ||
| match result { |
There was a problem hiding this comment.
we could probably combine these two.
| metrics: &Arc<StatsdClient>, | ||
| ) { | ||
| // Do the check before matching so that the read lock can be released right away. | ||
| let result = filter.read().await.fetch_new_settings(storage_client).await; |
There was a problem hiding this comment.
Ah, I didn't know that calling fetch_new_settings(&self) would also drop the lock guard from the current scope. TIL!
| } | ||
| Ok(()) | ||
| self.last_updated = Some(chrono::Utc::now()); |
There was a problem hiding this comment.
Instead of the current timestamp, we might want to store the last modified timestamp of the remote file to avoid some subtle ambiguities because filter::update() might be done significantly after filter::fetch_new_settings() (e.g. due to a long wait for the write lock). If there is any new update published between filter:fetch_new_settings() and filter:update(), Contile will be unable to pick it up (lost-update) at the next update check due to that obj.updated < self.last_updated check in fetch_new_settings().
It's quite subtle, aslo we're not making that much publish at the moment, so this can be handled later with a follow-up.
also fix a bug in the updater's handling of all_include_regions
DISCO-2203 & DISCO-2204
Closes #502
Closes #503