Hi Keith -
thank you for the review. I'll take a look at this first thing on Monday.
ginnie
On 07/16/10 02:18 PM, Keith Mitchell wrote:
On 07/16/10 12:46 PM, Ginnie Wray wrote:
Hi -
I've posted the python logging files for code review.
http://cr.opensolaris.org/~ginnie/install_logging_pyfiles/
I would like feedback by July 27th, COB.
Thanks,
ginnie
_______________________________________________
caiman-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/caiman-discuss
Hi Ginnie,
I spent a lot of time on this review, as I feel any CUD component
deserve a detailed review. For the most part, I think my comments are
"clean-up" related - making code easier to read, more flexible for the
long-term, etc. The only main implementation component I have concerns
about is the building of a ProgressLogRecord to not have a levelno,
and the amount of extra code that results from that.
(I also plan on reviewing the C portion early next week)
- Keith
Hi Ginnie,
usr/src/lib/Makefile:
Copyright: Should be 2007, 2010
Ordering of entries in COMSUBDIRS is backwards compared to SUBDIRS (in
the latter, XXX_pymod entries come after XXX). Could you fix this for
both the logging module and errsvc (assuming there's not some obscure
Makefile related reason they have to be in the opposite order, of
course).
install_common_vals.py:
These values can be defined in install_logging.py as module or class
variables, and this file removed.
install_logging.py:
Naming nit: Given that this file goes under
vendor-packages/osol_install, the "install" part of install_logging is
redundant.
General: Rather than stripping out "level" from Progress Records and
splitting logic paths down those lines, perhaps it would be better to
simply have the 'level' for Progress Records be <MAXINT> (or some
other large value that would pass all logging level checks). This
would let them fit in with the logging paradigm more smoothly, and
remove the need to override callHandlers in line 339.
57-58: Function isn't needed.
81: This seems like a stub that will be completed later, as preamble
isn't used anywhere that I can tell as of yet?
101-102: If the error is not 17, I imagine there will be a further
error raised by the __init__ call below, so printing the traceback
seems redundant. Need to either error out here, or simply pass
(regardless of the error code) and let the below __init__ call raise
exceptions if things aren't set-up right.
103-104: In what scenario does the try clause raise a TypeError?
109: I'm concerned that the need to override the "format" method of
the default FileHandler reduces the extensibility of our module -
would something similar need to be done for any other of the default
Python logging module's handler classes? If so, that's a bit of a
stopper in terms of extensibility. I think we can find a way around
this; perhaps by having a default formatter that itself contains two
separate format strings - one that gets applied to records with a
progress attribute, and one that gets applied to records without
progress data.
149: Should this be a subclass of logging.LogRecord? If not, should we
be mimic-ing more of the functionality (in particular, setting things
like self.created (time stamp), self.thread, self.threadName, etc.?
226-228: I think this could be simplified by weighting the progress
value at creation of the progress record, rather than upon emission of
the record (if necessary, ProgressRecord could be updated to store
both the original value and the weighted value). This would also
change it so that InstallLogger is dependent on
InstallEngine.get_weighted_progress(), rather than ProgressHandler -
which would allow for more long-term flexibility in terms of defining
classes that can receive/send progress. Along this same vein, I'd
suggest shifting how the instance of the install engine gets set - let
InstallEngine._init_logging set the engine instance (where the engine
instance is a class variable of InstallLogger). This provides cleaner
breakage between the modules, and allows for better management of
dependencies between the two - as well as providing a convenient way
for the unit tests to insert a mock engine (thus allowing the unit
test file to avoid having any need for the InstallEngine proper).
258: default_log and base_name - appear to be read-only properties.
May be better off removing them entirely and simply referencing the
constants they're initialized to.
259-262: Similarly, if these are intended to be constants, I'd
recommend having them be at the class or module level rather than the
instance level.
251: Default logging levels look strange - I'd expect this to either
be INFO or NOTSET, and the value passed to logging.Logger.__init__ to
be the variable "level" instead of hard-coded to a different default
value than InstallLogger.__init__.
267: Should also use the variable "level" to make changes consistent.
270, 290: These removeFilter calls seem unnecessary.
307: overzealous except clause
355: pep-8 nit: no spaces around '=' in keyword arguments
Function seems inflexible for future extension (no support for, for
example, transferring all registered FileHandlers' log files to a
single destination folder).
363: As destination is transient, there's no need to set
self.destination here - that'll just lead to stale data and confusion
during debugging. (This goes for many other places as well - no need
to store self.local_function_variable, just use the local variable
directly)
370: Printing to stderr here seems like the wrong choice - this will
cause problems with both text install (display will be distorted) and
gui install (nothing will get seen). It'd be better to let the
exception propagate and let the caller decide what to do (ignore, send
a message to the user, try again, etc.). Note: I think printing to
stderr is fine during initialization stages, as there's may be no
other reasonable method for communication.
374: More of a design consideration, so perhaps it's too late, but
would "shutdown" be a more appropriate name for this function?
378, 395: Similarly, printing to stderr seems like a bad idea here as
well. Perhaps this function could return a value (a list of files, for
example) that the caller could then use as needed?
387, 390: These AttributeErrors should be avoidable with use of
"hasattr(object, attr)"
install_engine.py:
Is this file temporary?
On a related note, the install engine will be under
osol_install.engine, so the imports in install_logging.py will need an
adjustment (although if the suggestion for shifting the dependency on
the engine is followed, the imports become completely unneeded).
install_logging_unittest.py
General: Suggest rename to test_install_logging.py (works better with
automatic recognition by nose).
I also think there is need for unit tests that target specific
functions of the classes defined in install_logging.py. The test cases
given cover the module as a whole, but don't drive down to the unit
level just yet.
24-51: I'd remove this, and add the test directory to
usr/src/tools/test/tests.nose, as that should be the preferred method
for running tests. (Once done, you should be able to generate a code
coverage report, which would be valuable to include with the review)
55: It should be possible to have this unit test file be completely
independent of the engine, if following the previous suggestions
regarding dependency on the engine. I believe the only necessary
action would be a call to logging.setLoggerClass(InstallLogger) as
part of setUp().
64-70: Simply setting self.test_logger to None should be sufficient.
73-80: The cleanup code will not fully cleanup in all cases. For
example, if line 74 raises an exception, line 77 won't get run, which
means that directory won't get cleaned up.
240-254: Have simpleprogressrecvr register a callback function or
something similar, instead of writing to a log file, to remove the
dependency on the filesystem (unit tests should strive for minimal
file I/O, though of course this module will be reliant on file I/O to
a degree when testing FileHandler). This would also then serve as a
good example for how gui/text installer might create an in-process
progress receiver to receive the socket updates and update the UI's
progress bar.
_______________________________________________
caiman-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/caiman-discuss
