Hi Jack,
On 03/05/2011 22:15, Jack Schwartz wrote:
Hi Darren and Matt.
Here are my comments on target_selection.py:
First of all, seems like a really nice piece of work!
Thanks ;) we worked hard on this one...
I am not an expert at this stuff and to become one would take me much
longer, so I did a medium-depth check given the time constraints...
210, 220: OptionS -> Options
Done
__handle_vdev, __handle_filesystem ,__handle_zvol,
__handle_pool_options, __handle_dataset_options are all the same except
for their logging messages. I see why this was done, as some entities
such as BEs or zpools need more processing and thus must have their own
handlers. However, the simple copy-only methods could be combined into
a generic, simple method and their logging messages displayed by the
method that calls the generic method, and
that would condense the code by ~30 lines.
What's 30 lines between friends ;)
I think the main thing really here is that it's obvious what each method is
meant to handle - if we combine some, then I think it will become less
obvious what to do if we need to do some specifics for a given item.
Plus, some of the logged messages are different ;)
452-457: Cleanup needed?
Done.
466, 1488: occured -> occurred
Done.
478, 491: This may have already been discussed, and this may be better
handled as a follow-on bug, but it seems overly-simplistic to calculate
swap and dump to be 1/2 the RAM size. These days some systems have so
much RAM that they have little or no swap. And as systems with
terabytes of RAM will work their way into the world, do we need swap and
dump that are half the size of RAM?
Agreed - but it's a common solution. This calculation has now been moved into
Target Controller so it's common between all the installers - so if we change
things in the future it will be easier.
This new method in target controller also takes into account the amount of
available space, adjusting the size of swap/dump if needed.
769: idenfifying -> identifying
782, 886: idenfifiable -> identifiable
Done...
786: amd -> and
795: ecah -> each
987: uniqely once -> once
1010: devince -> device
Done to the above.
1028: Can pull start_vdev_key assignment out of the loop.
Done.
1035: Not sure why a dict is needed here. A list should do.
uniq_vdevs = []
for vdev in zpool.children:
if isinstance(vdev, Vdev) and not vdev in uniq_vdevs:
uniq_vdevs.append(vdev)
return uniq_vdevs
I don't think you can use a list comprehension though, since the check
would need to access the list you are building, i.e. the "not vdev in
uniq_vdevs" of the below:
uniq_vdevs = [vdev for vdev in zpool.children if isinstance(vdev, Vdev)
and not vdev in uniq_vdevs]
A dictionary, is easier, I've changed to code to read as simply:
vdev_redundancies = dict()
for vdev in zpool.children:
if isinstance(vdev, Vdev):
vdev_redundancies[vdev.redundancy] = True
return vdev_redundancies.keys()
this uses the uniqueness guaranted by a dictionary to ensure things are
unique.
It's also faster (although it's really nominal here) to access dictionary keys
due to the use of hashes over searching a list using the 'in' operator.
1088: identifyable -> identifiable
1134: parition -> partition
1145: specifiction -> specification
Done
1155: Will a maintainer in the future know what TI stands for? I would
spell it out.
I really hope so :) But I've spelled it out in some cases.
1184: Can make this elif
Done
1494: nit: change name of __create_temp_logicals() to
__create-temp_logical() since only one is being created.
Not quite true - everything being created in there is a 'logical' as defined
by install_target - hence the logicals...
1631: looks like a personal comment. "I think..."
Removed.
1689: nit: s/have no children/have children/
1704: s/ to install too / to install to /
1778: somehoe -> somehow
1820: excpetion -> exception
many places: existant -> existent
All done.
Throughout the file (example: 190, 270): \ not needed with () spans
multiple lines.
Fixed loads :)
Thanks,
Darren.
