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
