Karen,
Here's my code review
Engine CR
-*-*-*-*-*-*-
usr/src/lib/install_engine/__init__.py
41-50: reorder your imports to be alphabetical
1,195, 201, 397, 532, 611, 857: remove unnecessary line
231-234: Remove the try/except and use
shutil.rmtree(self._tmp_cache_path, ignore_errors=True)
250: align with 249 (directly under "Wrong logger ….)
260-268: why not use the @property and @setter function decorators?
@property
def dataset(self):
return self._dataset
@dataset.setter(self):
<code>
383: add a space before 'has'
895-897: indent to match non comment code indentation levels
912-913: move above the for-loop in 909-911
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>
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>
960, 964: is there any reason these can't be @property?
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.
----------------------------
checkpoint.py
1, 102: remove unnecessary line
27-30: alphabetize imports and add an empty line after 29
----------------------------
checkpoint_data.py
1: remove unnecessary line
27-40: alphabetize imports and add an empty line after 34
74: use i += 1
----------------------------
I'm skipping the code review of the tests for now. Out of curiosity,
what's your coverage percentage?
----------------------------
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.
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.
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
