Fix broker properties not getting updated on Configuration change #711
Conversation
Move the cdi::Kind abstraction so that updates to broker properties can be reflected here. On Configuration reconciliation if request already exists, update broker properties Fix project-akri#705 Signed-off-by: Nicolas Belouin <[email protected]>
diconico07
requested review from
bfjelds,
kate-goldenring,
jiria,
Britel,
romoh,
adithyaj and
johnsonshih
as code owners
| fn device_to_instance( | ||
| &self, | ||
| dev: &DiscoveredDevice, | ||
| extra_device_properties: &HashMap<String, String>, |
There was a problem hiding this comment.
Why are you passing in properties instead of reading them from self.extra_device_properties?
| impl DiscoveryHandlerRequest for DHRequestImpl { | ||
| fn get_instances(&self) -> Result<Vec<Instance>, DiscoveryError> { | ||
| async fn get_instances(&self) -> Result<Vec<Instance>, DiscoveryError> { | ||
| let properties = self.extra_device_properties.read().await; |
There was a problem hiding this comment.
Same point here: could this skipped and instead let the called function (device_to_instance) access the properties? Or is the concern wanting to avoid too many reads? It is RWLock, so reads should be cheaper. I could also see how your strategy is trying to avoid a racecase where some containers have properties that were written after the initial call
There was a problem hiding this comment.
acquiring the lock is an async operation, you cannot do this from within the map call (same reason for not doing it in device_to_instance).
But in the end I find it better anyway, as it means we have a consistent set of properties for the entire run
There was a problem hiding this comment.
device_to_instance could be made async but i am aligned on wanting the consistent set of properties
|
/version patch |
Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
What this PR does / why we need it:
Move the moment we move to
cdi::Kindabstraction so that updates to broker properties can be reflected here.On Configuration reconciliation if request already exists, update broker properties
Fix #705
Special notes for your reviewer:
If applicable:
cargo fmt)cargo build)cargo clippy)cargo test)cargo doc)