Hi Keith,
Thank you for the code review. Please see my responses to your comments
inline.
On 09/23/10 11:56 AM, Keith Mitchell wrote:
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.
Fixed.
errsvc.py:
101: Check for "BaseException" here.
Fixed.
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).
Unfortunately, we can not do it in this case. It will create a circular
dependency between __init__.py
and CheckpointData.py. __init__.py needs to import the CheckpointData
object from CheckpointData.py,
but CheckpointData.py needs to import INSTALL_LOGGER_NAME and
get_argspec() from __init__.py.
So, looks like we really can not get rid of this file. We can give it a
better name. Do you have any suggestions?
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.
While I was writing the code, I implemented this function as __eq__, but
I don't seem to be
able to get the comparison to work. I found that it works
if I do it as __cmp__. Now that you mentioned it, I looked at it
again. After some search, I found that since I am
doing the comparison using "!=", the __eq__() function doesn't get
called. The __ne__() gets called instead.
If I don't have the __ne__() defined, the __cmp__() function is called.
That's why __cmp__() works for me.
Now, I restored the __eq__() function I had, and implemented a __ne__()
to return "not self.__eq__()".
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.
That makes sense. I will remove "loglevel" from the comparison.
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)
good idea. Fix as suggested.
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)
Change as suggested.
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)
That's a good suggestion. It will certainly make the code easier to
read since it will be easy to tell which arguments
are passed to the checkpoints, and which are used by the engine. I will
make the change.
370: Nit: Move this to line 379, just before the 'for' loop where it's
used.
Changed.
666: So, why use a Decimal but then cast it to an int?
cp_data.prog_est_ratio is a floating point number. So, the
normalized_prog will be a floating point value
if it is not cast to an int. I think it will be easier to return the
normalized progress
as an int instead of a floating point number.
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.
Agreed. Fixed as suggested.
782: Nit: No parens around int.
Fixed.
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.
If the value is an integer, you don't need to cast to string first. If
the argument for the Decimal() constructor
is a floating point number, then, it needs to be converted to string.
a=1
b=1.1
decimal.Decimal(a)
Decimal('1')
decimal.Decimal(b)
Traceback (most recent call last):
File "<stdin>", line 1, in<module>
File "/usr/lib/python2.6/decimal.py", line 649, in __new__
"First convert the float to a string")
TypeError: Cannot convert float to Decimal. First convert the float to
a string
decimal.Decimal(str(b))
Decimal('1.1')
For line 833, I think I will keep the cast to string, just in case
get_progress_estimate() returns a non-integer
for some reason. For line 837, I will change to Decimal(0).
948: Use "if InstallEngine.TMP_CACHE_ENV in os.environ" rather than a
try/except block.
Fixed.
1093: Nit: Trailing '\' not needed, due to surrounding parens.
Fixed.
Thank you again for the code review.
--Karen
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
