Hi Drew,
Thank you for the code review. Please see my responses inline.
On 09/23/10 09:43, Drew Fisher wrote:
Karen,
Here's my code review
Engine CR
-*-*-*-*-*-*-
usr/src/lib/install_engine/__init__.py
41-50: reorder your imports to be alphabetical
Fixed.
1,195, 201, 397, 532, 611, 857: remove unnecessary line
Fixed.
231-234: Remove the try/except and use
shutil.rmtree(self._tmp_cache_path, ignore_errors=True)
Fixed.
250: align with 249 (directly under "Wrong logger ….)
Fixed.
260-268: why not use the @property and @setter function decorators?
@property
def dataset(self):
return self._dataset
@dataset.setter(self):
<code>
Changed to use @property and @setter functions as suggested.
383: add a space before 'has'
Fixed
895-897: indent to match non comment code indentation levels
Fixed.
912-913: move above the for-loop in 909-911
Fixed.
924-928: You have an @property decorator but still have "get_" in the
name of the method. Can you change that to:
@property
def completed_checkpoints(self):
<code>
Actually, I removed this function since it is no longer used.
947-952: Don't use try/except as a control structure. Check for the
existence of an environment variable explicitly:
if InstallEngine.TMP_CACHE_ENV in os.environ:
if not os.path.exists(InstallEngine.TMP_CACHE_ENV):
os.makedirs(InstallEngine.TMP_CACHE_ENV)
else:
<prefix code>
Good idea. Fixed.
960, 964: is there any reason these can't be @property?
Those can not be @property because we need to pass the "cp_name"
variable as an argument to compute the results.
989, 1049: be consistent with your docstring triple quotes. In every
other method with multiline comments, you end with the triple quotes
on their own line.
Made them all consistent now.
----------------------------
checkpoint.py
1, 102: remove unnecessary line
Fixed.
27-30: alphabetize imports and add an empty line after 29
Fixed.
----------------------------
checkpoint_data.py
1: remove unnecessary line
Fixed
27-40: alphabetize imports and add an empty line after 34
Fixed.
74: use i += 1
Fixed.
----------------------------
I'm skipping the code review of the tests for now. Out of curiosity,
what's your coverage percentage?
Here are the numbers. I only copied and pasted the ones relevant to the
engine.
Name Stmts Miss Cover Missing
----------------------------------------------------------------------
solaris_install.engine 465 38 91% 107, 110,
113, 237, 243, 340, 344, 381, 508-509, 528-540, 747, 777-788, 892, 904,
909, 943-947, 983, 992
solaris_install.engine.checkpoint 17 2 88% 67, 84
solaris_install.engine.checkpoint_data 96 6 93% 63, 127,
130, 133, 170-172
solaris_install.engine.install_common 13 0 100%
----------------------------
Larger comments:
I've already talked this over with Keith and I think we should
consider moving all of the engine code away from threading and to use
the multiprocessing module instead.
The primary reason I suggest this, is for canceling running
checkpoints. WIth threads, and POSIX threads in general, you have no
control over the thread. You can very strongly suggest to the thread
that it should end, but you can not explicitly kill threads outright.
Yep, I understand this limitation.
Multiprocessing uses a subprocess / fork / exec / wait framework which
gives us 100% control over the process running. It would also allow
the engine cancel checkpoints instead of relying on the checkpoints
themselves to implement a cancel method.
That's one item we need to discuss and get other people's opinion. With
the current model of having the engine
suggest the checkpoint should quit and trust that the checkpoints will
behave and quit at it's earliest convinience,
the engine really has no control over whether the checkpoint. The good
thing about doing it this way is that the checkpoint
can come to a "good" stopping point and quit. The disadvantage is of
course that the engine has no control
over what a checkpoint does.
If we change to use the multiprocessing module like you suggested, the
engine can kill a checkpoint at anytime.
However, that might lead to a problem with the system being left in an
unknown state. I don't know whether
that's a good trade off or not.
Thanks,
--Karen
The good news is that the multiprocessing module uses a lot of
similarly named methods to execute things in a very similar way to
threading so the changes to the code should be minimized.
We can discuss this outside of this CR, if you'd like.
On 9/22/10 5: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
