Hi Ginnie,
Responses below.
On 07/20/10 03:00 PM, Ginnie Wray wrote:
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?
From a "what makes sense to me" standpoint, I'd prefer to keep
constants as close to the data they're tied to. They should be
referenceable from the unit tests regardless. I hadn't considered the
remote aspect, but that's a good point to consider. Although I don't
think a remote consumer would have much use for HOST='localhost', or the
default paths to the log files.
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.
Ok.
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.
Correct, but the layout of that except block has all OSErrors getting
ignored, and those with an error 17 get printed out. I think the
printing can be skipped. For non-17 errors, the exception should be
re-raised (or ignored, and just let FileHandler see if it can handle it.
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.
I assume you're referring to the "mode" keyword. Since the makedirs call
will be hardcoded to a mode of 0777, this shouldn't raise a TypeError
ever, so that except clause can be removed.
When looking at this, I also realize that on line 105, you have the mode
keyword arg of FileHandler.__init__ hardcoded to "a" instead of passing
the value sent in from line 95 by the caller. This call on line 105
should be changed to:
logging.FileHandler.__init__(self, filename, mode=mode,
encoding=encoding, delay=delay)
to ensure proper passthrough.
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.
But the code here only affects FileHandlers, which means it will need to
be duplicated for any other Handler we want to use.
As an alternative, I think it'd work well if ProgressFormatter
sub-classed the logging.Formatter, and then had two separate "fmt"
styles (one for records with progress, one for those without). For those
records with progress, do as you've done in the current
ProgressFormatter.format. For those without, call
logging.Formatter.format() on the record instead.
Then, any handler would have a way of logging messages with progress, if
they so desired, by simply using a ProgressFormatter - without needing
to sub-class themselves to handle it.
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.
Oops. I wrote the review over the course of a couple days, and I'd meant
to come back and add more detail here, but it looks like I missed that
before hitting send.
PEP-8 requests that an "except" clause should always specify exactly
what exception it's trying to handle - they shouldn't be bare (with the
sole exception of top-level applications handling uncaught exceptions by
printing an appropriate message before exiting).
This code seems to be taking a ProgressHandler; if that ProgressHandler
has a filename, then it's creating a FileHandler to point to that same
file, and adding it to the InstallLogger. I'm guessing the except clause
gets hit when the handler's filename is set to None.
Rather than expecting the caller to know that in order to create a
ProgressHandler that logs to a file, they need to create an instance of
ProgressHandler (which is a subclass of SocketHandler, and would be
expected to send data to sockets) and set the filename attribute, and
then pass that into logger.addHandler to generate a FileHandler that
does what they want, I'd expect the caller to simply create the
FileHandler (with appropriate filters and formatters for progress
records), and pass it into addHandler. If necessary, a
FileProgressHandler that sets these filters/formatters to appropriate
defaults would be a better approach.
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.
Yeah, I couldn't think of a reasonable need to transfer more anyway.
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.
It's fine to leave it as is then.
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
You're welcome. Let me know if you'd like to discuss the testing at all;
this module is probably a little more difficult to set-up isolated tests
for given its nature.
- Keith
_______________________________________________
caiman-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/caiman-discuss
