Hi Karen,
I only partially reviewed, but here are my comments (I skipped the
portions that I wrote since I'm probably biased for those!)
Thanks,
Keith
usr/src/lib/Makefile:
Alphabetically, install_engine should be first.
errsvc.py:
101: Check for "BaseException" here.
install_common.py:
I'm not a fan of ambiguous names like "common" and "utils" for files.
Too easy for them to become dumping grounds for random code. Is there
any reason not to put INSTALL_LOGGER_NAME and get_argspec in
engine/__init__? I think most consumers of that variable will have the
engine anyway (and by virtue of importing
"solaris_install.engine.install_common" they'd be importing
"solaris_install" and "solaris_install.engine", so there's no memory /
performance gain by separating this).
checkpoint_data.py:
113: I believe this should be __eq__, as __cmp__ is used to define both
equality and sorting mechanisms. An AttributeError may be raised if a
comparison of CheckpointRegistrationData == "some string", so this
should be caught and 1 returned if that happens.
I also think "loglevel" shouldn't be checked - it'd be quite useful to
be able to change the loglevel between executions to get higher/lower
output, without invalidating the ability to resume from that checkpoint.
engine/__init__.py:
37-39: Given all the imports from decimal, I'd say just "import decimal"
and use "decimal.decimal_context()" etc. where it's referenced
(particularly since it's only used a few times)
211: Should use "self.dataset = dataset" so that the setter property
(set_dataset) is used to properly cast this into a zfs.Dataset object as
needed. (To make pylint happy, you may have to do "self._dataset = None;
self.dataset = dataset", odd as that looks)
270: I wonder if it would be better to define this function signature as:
def register_checkpoint(self, checkpoint_name, module_path,
checkpoint_class_name, insert_before=None, loglevel=None, args=None,
kwargs=None):
This separates the arguments to the register_checkpoint function from
the arguments that will get passed to the checkpoint itself. It means
that callers will have to update themselves to construct and pass in a
tuple for args and a dict for kwargs, but that seems to be a cleaner
separation. (This would be similar to how one passes args/kwargs to a
threading.Thread:
http://docs.python.org/library/threading.html#thread-objects)
370: Nit: Move this to line 379, just before the 'for' loop where it's used.
666: So, why use a Decimal but then cast it to an int?
730-731 & 761-769: As discussed, these need to be in a single 'with'
block. cancel_checkpoints() needs to know that the __currently_executing
checkpoint which it signaled to cancel is in fact the last checkpoint
that gets executed, so _execute_checkpoints needs to check the
_cancel_event flag at the same time as it updates __currently_executing.
782: Nit: No parens around int.
833: I think it should be possible to just do "Decimal(prog_est)"
without the intermediate cast to string.
837: I think this could be "Decimal(0)" though I'm not sure how much of
a difference it makes.
948: Use "if InstallEngine.TMP_CACHE_ENV in os.environ" rather than a
try/except block.
1093: Nit: Trailing '\' not needed, due to surrounding parens.
On 09/22/10 04:00 PM, Karen Tung wrote:
Hi,
The install engine implementation is ready for code review. The webrev
is at:
http://cr.opensolaris.org/~ktung/cud-engine/
All the Makefile related changes made in the usr/src/ and usr/src/lib are
temporary changes needed for the gate to build cleanly. They might
be the same changes the logging and Data Object Cache projects made.
When those 2 projects integrate into the slim_source gate, I will
update the
Makefiles as needed before integration.
All tests implemented in the test directory of the engine code are
executed
successfully. All the engine API are also being used by the
Distribution Construction
project as they move DC over to using CUD.
I would like to get at least 2 reviewers. If you plan to review all
or part of the
code, please email me privately so I can make sure the code get good
review coverage.
Please send all your comments by COB 10/6/2010.
Thanks,
--Karen
_______________________________________________
caiman-discuss mailing list
caiman-discuss@opensolaris.org
http://mail.opensolaris.org/mailman/listinfo/caiman-discuss
_______________________________________________
caiman-discuss mailing list
caiman-discuss@opensolaris.org
http://mail.opensolaris.org/mailman/listinfo/caiman-discuss
