Hi Martin,
Please update copyright in test_logger.py
Other changes look good to me.
Thanks,
--Karen
On 02/23/11 12:27 PM, Martin Widjaja wrote:
All,
Quick review on this is appreciated. Hopefully this is it (in
hindsight I should've stayed away from logger bugs.. ;-)
- Removed source arg from transfer_log and the corresponding
filehandler.transfer_log.
- Modified logger test for source>dest test
Webrev: http://cr.opensolaris.org/~widjaja/logger-fix/
<http://cr.opensolaris.org/%7Ewidjaja/logger-fix/>
Bugs:
7006785 <http://bugs.opensolaris.org/bugdatabase/view_bug.do?bug_id=7006785>
some of the logging unittests are broken
7012566 <http://bugs.opensolaris.org/bugdatabase/view_bug.do?bug_id=7012566>
InstallLogger.close() is broken
Martin
On 2/22/2011 9:02 PM, Martin Widjaja wrote:
Hi Karen,
Thanks for the comments. Please see below...
On 2/22/2011 2:15 PM, Karen Tung wrote:
Hi Martin,
Please see my comments below.
On 02/22/11 13:13, Martin Widjaja wrote:
Here's what I am proposing:
- FileHandler.transfer_log to have source as keyword (optional) arg
to provide flexibility for people to use this
- FileHandler.transfer_log to throw an exception if it currently
has an open reference to a file different from what's passed into
the source
I do not think there's any value to pass source as a keyword
argument, even if it is optional.
When a FileHandler is created, the file it needs to handle is
provided as a required argument.
So, the FileHandler class should always have a reference to the
"source" file. It's unnecessary to pass it in as an argument.
Yes, that's what makes sense for such thing as filehandler, and I
even thought this was kind of strange, but based on some of the
earlier comments that this was a needed functionality, I didn't want
to make so much changes that would break others. Thus, to achieve the
needed balance, though not ideal, I have decided to go with whatever
was already in place and just fix the immediate issue/bug.
Further, I don't see how it would be useful to allow people to pass
in a source, and then, raise an exception
if the source passed in differs from the file the FileHandler is
working with. This means if people pass in
a source that's different than the file it specified when it created
the FileHandler, we will get an exception.
So, there's really no flexibility provided.
Yeah, thinking about this more, you are right. If the goal was to
provide flexibility, this exception is not going to work. I guess
earlier I was reacting to Keith's comments on the problem when
someone passes in a file that's not the original file referenced by
the descriptor.
This is still combining the two functionalities somewhat into
FileHandler but with explicit error handling mechanism to prevent
people from doing whatever they can, but at the same time allowing
ICT functionality which is already using FileHandler.transfer_log
to continue to work until a later time we decided to move this
somewhere.
We need to provide a transfer_log function in InstalllLogger to move
the default log to the provided location
because the application have no control over where is the default
log, what's the default log's name...etc..
So, InstallLogger.transfer_log() is provided to move the default log
to a location specified by the application.
Unlike the default log, the application itself creates the other
FileHandlers providing a name.
So, the applications know exactly where those log files are. When
the application is done
with logging, it can use normal Python functionality to move those
log files.
I do not see any value in providing this functionality in the
InstallLogger.
Yes, I guess this all makes sense now. Much earlier (on Friday) I
didn't realize that FileHandlers were actually being used directly by
different applications, and I thought that apps only call this
through transfer_log method.
I guess I misunderstood the use case for this Filehandlers, though
Alok did mention DC functionality would break, but it didn't quite
click until today when someone made that comments.
Anyway, I'll remove that functionality and I'll coordinate with
Ginnie/Darren/Matt to make sure they are not going to be affected for
their future work (I am assuming they don't have this call yet
because this is something that's not there).
Thanks,
Martin
Thanks,
--Karen
I am not familiar with the transfer module, so if anyone (Keith and
Ginnie in particular?) thinks this can be better off somewhere and
we can shut source argument from filehandler forever, then we
should discuss further.
Ginnie - Is the usage of this transfer_log pretty widespread in the
ICT area?
Thanks,
Martin
On 2/22/2011 11:28 AM, Keith Mitchell wrote:
If that is the functionality needed, then what really is needed is
two separate functions:
1) To transfer an arbitrary, *closed* and completed log file from
one location to another. A glorified shutil.copy, as it were.
2) A function on the FileHandler that closes its current log file,
moves it, and re-opens it for appending, to allow for continued
logging to that file.
Combining the two into a single function that tries to do both is
only going to lead to odd bugs and inconsistent log files.
#2 is how FileHandler.transfer_log() is currently implemented. Its
behavior can't be changed without breaking DC's use case
#1 sounds an awful lot like what the transfer module does...
There's certainly some commonality - a helper function that does
the "make dirs as needed, then move the file" portion might be
warranted.
- Keith
On 02/22/11 10:28 AM, Virginia Wray wrote:
Hi -
We do need a means of transferring a source file to a
destination. The icts have that functionality currently, and I've
had a conversation with Darren and Matt around the need for
moving various files to a different destination as part of the AI
work.
Rather than making the transfer_logs funtionality tied only to
the DEFAULTFILEHANDLER, could we make it more robust so that it
could handle any source/destination file transfer? I think
something along those lines was part of the original
transfer_logs code.
thanks,
ginnie
On 02/22/11 10:56 AM, Keith Mitchell wrote:
[Re-send, first attempt failed...]
On 02/22/11 09:52 AM, Keith Mitchell wrote:
Hi Martin,
I'm still not sure adding the source parameter is the correct
route. First, source should be a keyword (optional) argument
here, if anything. But more importantly, consider the side
effects of passing in source:
A FileHandler, fh, has an open reference to file X
Someone calls fh.transfer_log(source=Y, destination=Z)
The transfer_log code will now:
- close the reference to X, meaning that the filehandler is now
no longer able to log to the place it's supposed to be logging to
- Copy Y to Z
- Continue logging by appending to Z.
This will lead to half of that FileHandler's log data in X, and
half appended to Z. There's a major disconnect there that will
be *very* confusing.
- Keith
On 02/21/11 05:27 PM, Martin Widjaja wrote:
Hi all,
Can I get a review of the logger bugs that are reported in the
following 2 issues:
7006785 <http://bugs.opensolaris.org/bugdatabase/view_bug.do?bug_id=7006785>
some of the logging unittests are broken
7012566 <http://bugs.opensolaris.org/bugdatabase/view_bug.do?bug_id=7012566>
InstallLogger.close() is broken
The webrev is available at:
http://cr.opensolaris.org/~widjaja/logger-fix/
<http://cr.opensolaris.org/%7Ewidjaja/logger-fix/>
This is a continuation of the fix I did for 7012566. As I was
unit testing my fixes, I ran into more issues causing several
unit test errors, so decided to do all the fix in one txn.
Note that I attempted to keep all the old test functionalities
such as transfer_log test with source specified, although some
of you mentioned to me this is not really needed any longer. I
thought that providing more flexibility to transfer_log (to
specify source) is a better fix than removing the test case
for it. Plus, not knowing the background story it seems to
make more sense for me to keep all these tests around for now,
at least.
Thanks,
Martin
_______________________________________________
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
_______________________________________________
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
_______________________________________________
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
_______________________________________________
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
