Hi Keith -
Thanks for the code review....see my response inline.
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).
Fixed.
install_common_vals.py:
These values can be defined in install_logging.py as module or class
variables, and this file removed.
I originally had them defined a globals, but then thought it would make
more sense to have
them in a separate file. I moved them when I was writing the unit
tests....because I needed to use the same variables in the tests. My
thinking was that for remote logging, we might want to have access to
some variables used by the logger, but not necessarily the whole logging
module.
Your feedback on this?
install_logging.py:
Naming nit: Given that this file goes under
vendor-packages/osol_install, the "install" part of install_logging is
redundant.
True. I was trying to differentiate if from the logging.py module. I
thought maybe that would get too confusing. Are you suggesting
osol_install.logging instead of osol_install.install_logging? Now that I
type it, it does look better. :)
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.
ok. I'll remove that. thanks.
81: This seems like a stub that will be completed later, as preamble
isn't used anywhere that I can tell as of yet?
Yes...Sanjay and I talked about this and we will be adding more for
remote logging as it is implemented.
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.
Here is my logic for setting this up. The FileHandler Class doesn't do
any checking for directory paths, and it is sporadic on creating them
and returns a traceback in some instances.
For example, if /var/tmp/install exists, and I want to create
/var/tmp/install/log/default_log, it will work because the log directory
doesn't exist. But, if I create /var/tmp/install/default_log, it fails
because I'm not creating a new directory in that scenario.
I didn't think the install logger should fail because the directory
didn't exist, so I added the os.mkdirs command to check for that. It
returns a traceback if the directory already exists (that is error 17),
and that seems like an error that can be ignored in this case.
103-104: In what scenario does the try clause raise a TypeError?
The pydoc for os.makedirs shows mode as an optional keyword, but it's
actually not a keyword; it's an arg that can be passed. My thought was
to catch that if someone passes that in incorrectly because of the
incorrect pydoc information.
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.
I'm not clear what your concern is here. I believe that what I'm doing
is what your suggesting.
The scenario I'm expressing in this code is the following...
When logging is invoked a default log file is created. If a Progress
Handler is invoked, there is a default progress log file created.
If a call to log a record to a file comes in the format method checks to
see if it's a progress log. If it is, it uses progress log formatting.
That's the only change I made to the Filehandler format method from the
logging.py module.
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.?
I'll take a look at that. I didn't do it because there wasn't any
overlap in the information passed in to the records.
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).
ok. This seems reasonable. Let me review it and see what I can come up with.
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.
Ok. I can do that.
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.
ok. I'll clean this up.
270, 290: These removeFilter calls seem unnecessary.
That might be an artifact from an earlier version that I somehow missed.
I'll review that.
307: overzealous except clause
No one has ever said I shouldn't be over-zealous.
355: pep-8 nit: no spaces around '=' in keyword arguments
Missed that one in my cleanup.
Function seems inflexible for future extension (no support for, for
example, transferring all registered FileHandlers' log files to a
single destination folder).
I actually had that in, but I had a discussion with Karen, and the
result was that we decided only the default files would be transferred.
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)
ok.
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.
ok. I'll fix this.
374: More of a design consideration, so perhaps it's too late, but
would "shutdown" be a more appropriate name for this function?
I don't have any strong feelings about this. I was following the
architecture doc. However, I have coded it that way, and that is the
info that has been propagated to test. I kind of hate to change things
on them.
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?
ok. I can add that.
387, 390: These AttributeErrors should be avoidable with use of
"hasattr(object, attr)"
ok. I'll give that a try.
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).
Actually, at the time, I thought I might need to include it for the
tests, but there is not reason not to use the osol engine.
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.
ok.
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)
ok.
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().
ok. I'll look at that.
64-70: Simply setting self.test_logger to None should be sufficient.
ok.
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.
ok. I'll look at that.
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.
ok. I'll take a look at that too.
Thanks again. I appreciate the feedback.
ginnie
_______________________________________________
caiman-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/caiman-discuss
