Re: [pbs-devel] [PATCH proxmox-backup] datastore: remove datastore from internal cache based on maintenance mode

["Hannes Laimer" <h.laimer@proxmox.com>]

Thanks for the review, will send a v2

On Thu Feb 29, 2024 at 6:13 PM CET, Gabriel Goller wrote:
> Tested this thoroughly and it works as advertised!
>
> Just some small nits inline:
>
> >  impl MaintenanceMode {
> > +    /// Used for deciding weather the datastore is cleared from the internal cache after the last
>
> Small typo: weather -> whether
>
> > +            // remove datastore from cache iff last task finished
> > +            // and datastore is in a maintenance mode that says it should be
>
> Did something get cut off here? The sentence doesn't sound correct...
>

I see where you're coming from, if the maintanance mode says it should
be clear from cache, since the maintenance mode defines weather it
should be or not. But I'll phrase this better

> > +            let remove_from_cache = last_task
> > +                && pbs_config::datastore::config()
> > +                    .and_then(|(s, _)| s.lookup::<DataStoreConfig>("datastore", &self.name()))
>
> `self.name()` is enough here, we don't need `&self.name()`
>
> > +    pub fn update_datastore_cache() -> Result<(), Error> {
> > +        let (config, _digest) = pbs_config::datastore::config()?;
> > +        for (store, (_, _)) in &config.sections {
> > +            let datastore: DataStoreConfig = config.lookup("datastore", &store)?;
>
> `store` is already a &String, we don't need to write `&store`
>
> > +            if datastore
> > +                .get_maintenance_mode()
> > +                .map_or(false, |m| m.clear_from_cache())
> > +            {
> > +                let _ = DataStore::lookup_datastore(&store, Some(Operation::Lookup));
>
> Same here
>
> >                              match operation {
> >                                  Operation::Read => task.active_operations.read += count,
> >                                  Operation::Write => task.active_operations.write += count,
> >                                  Operation::Lookup => (), // no IO must happen there
> >                              };
> > +                            updated_active_operations = task.active_operations.clone();
>
> You can remove the `.clone()` call here
>
>
> Consider:
>
> Tested-by: Gabriel Goller <g.goller@proxmox.com>
> Reviewed-by: Gabriel Goller <g.goller@proxmox.com>
>
>
> _______________________________________________
> pbs-devel mailing list
> pbs-devel@lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel