Hi Karen -
Thanks for the review. See inline.
ginnie
On 07/22/10 17:48, Karen Tung wrote:
Hi Ginnie,
Keith and Darren covered a lot of comments I wanted to make. I
tried to not repeat what's already been discussed. However,
if I repeat their comments, please just point me to your responses to them.
* General comments:
- Keith mentioned in his comments about defining a "level" for the progress
records, so the progress handler can fit more smoothly within the logging
paradigm. I thought that's a great suggestion, and I completely
agree with that. I haven't seen your response to that comment yet,
so, I am wondering what you think.
I somehow glossed over that comment when I replied to his review. I
agree, and I will look at implementing that. I think it will clean
things up a bit.
Specific comments:
----------------------
usr/src/lib/install_logging_pymod/install_logging.py
- lines 43-45: Just curious, why do you need to define an
InstallLogException that is not used anywhere else in the code,
just so that it can be subclass by the LogInitException.
Can the LogInitException() just subclass from Exception?
I can take that out. I'll modify the LogInitException.
- line 177: I don't think there should be default values for the host
and port arguments. I think host and port should be required arguments.
The ProgressHandler is a client in the server/client model.
The application must start the server, so the ProgressHandler's __init__()
can connect to it in "createSocket()". Since the application must start
the server, it must know the host and port information, so, I think
those 2 values should be required arguments.
That's a good point. I agree.
- line 182: Looks like tmp_progress_holder variable is only used in the
emit()
function. Is there a reason why it needs to be defined here.
If we have more than one progresshandler, the record.progress value
needs to be preserved. In the emit method for the progress handler (lns
224 - 246), the weighted progress is calculated. Then the
record.progress variable is set equal to that and sent to be published
to the logging output. If there is more than one ProgressHandler, the
whole cycle will happen again with the same record. If I don't put the
original value back in, it doesn't get calculated correctly for
subsequent progress handlers (ln 242). It looks like this instead:
ProgressHandler 1:
PROGRESS REPORT: progress percent:100 this is a progress message percent 100
PROGRESS REPORT: progress percent:50 this is a progress message for
installlogger 50
Progresshandler 2:
PROGRESS REPORT: progress percent:0.01 this is a progress message
percent 100
PROGRESS REPORT: progress percent:0.005 this is a progress message for
installlogger 50
Let me know if you think there is a cleaner way to do this.
- line 193: is there a reason why the "from install_engine...." line is
here instead of up at the top with the rest of the import statements?
I had seen this done in some Python examples, but Keith had suggested
instantiating the engine in the InstallLogger init, so I will be
moving it anyway.
- lines 236-239: TypeError and AttributeError are raised
due to something is wrong in computing the weighted progress
or formating the record. By capturing these 2 errors and printing
them out, we will move on to sending the message, which might not
be correct. Is that intentional?
That is probably not what I want to do. Thanks for catching that.
- line 274: is it really necessary to create a default progress log file?
When the logger initialized, there's no default progress logger. So,
who is ever going to write to the default progress log? When they
eventually add a ProgressHandler, and if they don't specify a filename,
the default progress log will never get written to.
I'll modify that so that a default progress log file is created when a
ProgressHandler is created.
- lines 374-397: I am not sure the value of printing out all
the log file names in the "close()" function. The application would
know all the log file names, since it is the one that added the file
handlers.
As for the location of the default log, if it's value were to be printed,
it would be more useful to print it when logging is initialized.
ok. I'll change that.
-------------
usr/src/lib/install_logging_pymod/test/install_logging_unittest.py:
- line 84: Did you forget to remove that line? :-)
oops.
- lines 89-97: In the "test_create_defaultlog" test, you just want to
verify that the default log is created. Why not just check it's
existence with os.stat() or something similar.
ok.
- lines 114-122: again, check existence of file using os.stat()
- lines 140-149: I think you can do all of these by calling filecmp.cmp()
ok.
Thanks for the review!
Thanks,
--Karen
On 07/16/10 12:46, 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
_______________________________________________
caiman-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/caiman-discuss
