Hi Ginnie,
I think that Keith has covered quite a lot of what I would like to say, so
I'll try avoid repeating anything he said here:
Add logging to osol_install/__init__.py
---------------------------------------
- I need to do it for the DOC too but we should be probably adding our modules
to the file:
usr/src/lib/install_utils/__init__.py
which gets installed into :
/usr/lib/python2.6/vendor-packages/osol_install/__init__.py
install_logging.py
------------------
lines 27,28:
- It shouldn't be necessary to include both of the imports:
import logging
and
from logging import DEBUG, NOTSET
In general you should really only import either one way or the other.
lines 39-40:
- Should these variables be "internal" to the module, rather than exposed, if
so, they should be prefixed with an underscore or two.
lines 67/68 - indent seems wrong.
line 71: if self.log_progress is a private variable, then prefix with
underscore(s).
line 86: is the storage of the preamble really needed if it's not used?
if you want to store it as private, then use underscores too.
line 109: Overriding format()
- I'm agree with Keith that overriding this method seems to be the wrong way
to do this.
Given that there is a 'formatter' attribute for this class, to me this would
seem to be the correct way to apply the output format - in otherwords the
ProgressFormatter, would be the default formatter for us - and it would be
the one to make the decision as to whether to output using a progress style
format or not.
In otherwords override it's ProgressFormatter()'s format() method rather
than the handlers and set the FileHandler formatter attribute to be the
ProgressFormatter...
line 168:
- Should 'This can be a remote socket of an on disk file.' read
as: 'This can be a remote socket or an on disk file.' (of => or)?
line 175 / 195:
- Is it '_default_filename' or 'default_filename' - neither appear to be
used...
line 193:
- This should probably be 'from osol_install.install_engine', or whatever the
correct package name is.
line 251-262:
- I don't totally agree with Keith on removing some of these and just
referencing the constants - some of the values are being calculated here,
but I do think that if they are read-only, and internal then they should be
suitably prefixed with underscores to avoid mis-use.
Certainly, if the value is not being calculated (i.e. a direct reference,
like self.destination is, then an instance attibute isn't needed).
Thanks,
Darren.
On 07/16/10 09: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
_______________________________________________
caiman-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/caiman-discuss
