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.
_______________________________________________
caiman-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/caiman-discuss
