On Apr 15 11:14, Ilias Tsitsimpis wrote:
> On Mon, Apr 14, 2014 at 05:11PM, Jose A. Lopes wrote:
> > On Apr 14 14:29, Ilias Tsitsimpis wrote:
> > > This property returns the nodes covered by a disk.
> > >
> > > Signed-off-by: Ilias Tsitsimpis <[email protected]>
> > > ---
> > > lib/objects.py | 17 +++++++++++++++++
> > > 1 file changed, 17 insertions(+)
> > >
> > > diff --git a/lib/objects.py b/lib/objects.py
> > > index 874951e..2a4290d 100644
> > > --- a/lib/objects.py
> > > +++ b/lib/objects.py
> > > @@ -537,6 +537,23 @@ class Disk(ConfigObject):
> > > # is sent to, and should not be persisted.
> > > ["dynamic_params"])
> > >
> > > + def _ComputeAllNodes(self):
> > > + """Compute the list of all nodes covered by a device and its
> > > children."""
> > > + nodes = list()
> > > +
> > > + if self.dev_type in constants.DTS_DRBD:
> > > + nodea, nodeb = self.logical_id[:2]
> > > + nodes.append(nodea)
> > > + nodes.append(nodeb)
> > > + if self.children:
> > > + for child in self.children:
> > > + nodes.extend(child.all_nodes)
> >
> > Doesn't this need to be done recursively through the children?
>
> You are right, it needs to be done recursively through the children. But
> this snippet does it already:
>
> for child in self.children:
> nodes.extend(child.all_nodes)
>
> 'all_nodes' is a property that is implemented using the
> '_ComputeAllNodes' function.
This has a lot of room of optimization. At each step, you create a
list, make it a set, and then a tuple. It's much better to just
create a single list and, at the end, make it a set, then a tuple.
Also, having a recursive function that goes through the property and
back to the function is pretty confusing and misleading. Just go
straight for a simple recursive function, which you would propably
would have to do anyway to fix the previous problem.
> > Also, why not
> >
> > nodes.extend(self.logical_id[:2])
>
> That would work too. I copied the above code from Instance's
> '_ComputeAllNodes' method so I didn't give too much though to it. Do you
> want me to change it?
Yes.
Given the extensive number of changes, please send the entire patch
again, not just the interdiffs.
Thanks,
Jose
> Thanks,
> Ilias
>
>
> > > +
> > > + return tuple(set(nodes))
> > > +
> > > + all_nodes = property(_ComputeAllNodes, None, None,
> > > + "List of names of all the nodes of a disk")
> > > +
> > > def CreateOnSecondary(self):
> > > """Test if this device needs to be created on a secondary node."""
> > > return self.dev_type in (constants.DT_DRBD8, constants.DT_PLAIN)
> > > --
> > > 1.9.1
> > >
> >
> > --
> > Jose Antonio Lopes
> > Ganeti Engineering
> > Google Germany GmbH
> > Dienerstr. 12, 80331, München
> >
> > Registergericht und -nummer: Hamburg, HRB 86891
> > Sitz der Gesellschaft: Hamburg
> > Geschäftsführer: Graham Law, Christine Elizabeth Flores
> > Steuernummer: 48/725/00206
> > Umsatzsteueridentifikationsnummer: DE813741370
>
> --
> Ilias Tsitsimpis
> OpenPGP public key ID:
> pub 4096R/25F3E3EE 2012-11-02 Ilias Tsitsimpis <[email protected]>
> Key fingerprint = 1986 21F5 7024 9B25 F4E2 4EF7 6FB8 90BA 25F3 E3EE
>
>
> Tiger got to hunt, Bird got to fly;
> Lisper got to sit and wonder, (Y (Y Y))?
>
> Tiger got to sleep, Bird got to land;
> Lisper got to tell himself he understand.
>
> — Kurt Vonnegut, modified by Darius Bacon
--
Jose Antonio Lopes
Ganeti Engineering
Google Germany GmbH
Dienerstr. 12, 80331, München
Registergericht und -nummer: Hamburg, HRB 86891
Sitz der Gesellschaft: Hamburg
Geschäftsführer: Graham Law, Christine Elizabeth Flores
Steuernummer: 48/725/00206
Umsatzsteueridentifikationsnummer: DE813741370
