M. Ranganathan wrote: > On Mon, Jun 2, 2008 at 10:41 AM, Damian Krzeminski <[EMAIL PROTECTED]> wrote: >> [EMAIL PROTECTED] wrote: >>> Project sipXecs >>> New Revision 12793 >>> <http://sipxecs.sipfoundry.org/ViewVC/sipXecs?view=rev&rev=12793> >>> Committer mranga (M. Ranganathan) >>> Date 2008-06-01 09:23:39 -0400 (Sun, 01 Jun 2008) >>> >>> >>> Log >>> >>> >>> Work related to Issue XCF-2510 >>> >>> Adds logging to the stack ( should be repackaged into sipx-commons.jar ). >>> Checkstyle fixes applied to the logging classes. >>> Fixes a bug with the NOTIFY. Has been manually tested. >>> >>> >>> >> Before I dive into details I have one general comment: sipXconfig is already >> indirectly initializing log4j. By changing log4j.properties we >> can control the format and logging level of nearly each of the ~10 libraries >> that we use. >> >> Even if jain-sip provides additional methods of initializing log4j I am >> pretty sure that with sipXconfig you do not have to use it. Did you try just >> to set javax.sip related loggers in log4j and see what happens? > > I would like to be able to directly set a log4j appender as has been > done in the code at present. The logging setup in jain-sip is a bit > broken but so long as it does the job, I do not want to invest too > much time tweaking on it. I have a way of dealing with multiple stacks > ( common scenario in containers ) and assigning each stack its own > logging stream (distinguished by stack name ). I would rather not use > a single logging stream for everything if I can avoid it. > Unfortunately, sixp follows a different approach ( fine because we > never have multiple stacks per jvm ) so I had to override the JSIP > logging framework and register a logger ( with the formatter that Woof > wrote and that I modified ).
I think that the more elegant way of sending the log events to multiple streams in log4j is by using nested diagnostic contexts. http://logging.apache.org/log4j/1.2/faq.html#3.1 But I realize it's probably not a discussion for this list. I'll see what I can do to make sipXconfig to work with current jain-sip logging implementation without too much wizardy. > >> Now some comments: >> >> --- a/sipXconfig/neoconf/src/org/sipfoundry/sipxconfig/sip/NotifyMessage.java >> +++ b/sipXconfig/neoconf/src/org/sipfoundry/sipxconfig/sip/NotifyMessage.java >> @@ -50,7 +50,7 @@ class NotifyMessage extends JainSipMessage { >> clientTx.setApplicationData(tad); >> clientTx.sendRequest(); >> EventObject eventObject = tad.block(); >> - if (tad.block() == null) { >> + if (eventObject == null) { >> throw new SipException("Timed out waiting for response"); >> >> >> This is a bug fix unrelated to logging. It should get its own commit. > > > OK. Wanted to avoid lots of little commits ( not withstanding the fact > that I made some little commits thereafter ). > >> >> * >> + * @author mranga >> + * >> >> >> I ask contributors not to use author tags in sipXconfig. It's the >> information stored in repository already. After code goes through a set of >> changes the @author info is not real anyway. And no-one should be >> discouraged from sending patches to any part of the code. > > Sure. Can you make your style checking tool catch stuff like that. I > use @author as a way of assigning responsibility on a file by file > basis but I will gladly follow your conventions. Saw it done elsewhere > in sipx and hence did the same. > Good suggestion. I'll add it. >> When I took your previous patch I split it in 2 parts and did not apply one >> of them. I wanted to take them only after we start using dnsjava. But this >> commit is re-introducing them. >> I doubt that was done on purpose. It nearly looks like some files got copied >> from some other place to your subversion repo or did not get merged properly >> after svn up. Unless I hear from you otherwise I'll remove those changes >> again: > > Thanks. > >> - SipStackBean.java should not reference DomainManagerImpl >> - SipServiceImpl.java should not have SipServerImpl > > I noticed that in the trace of a unit test, there was an attempt to > set SIpServer and it did not succeed because of "unimplemented > method". So I threw that in to get rid of the error. I had assumed it > was an omission on your part :-). > > >> >> --- a/sipXconfig/web/etc/log4j.properties >> +++ b/sipXconfig/web/etc/log4j.properties >> >> + >> +log4j.category.javax.sip=debug >> + >> >> This is fine for testing. But by committing that you are switching the >> logging level to debug in the production code. > > > OK. accepted. Until the code is deemed production, can we leave it > there? It only affects the sip stack. No problem. I'll add a FIXME note there. > >> I'll probably move woof's classes related to logging to a separate package. >> Should we use this appender for everything in the log file or just for SIP >> messages? > > > Only for SIP messages. However, please use log4j and only log > synchronously into that common file. It would be a very good idea to > move all the logging classes ( not just the Formatter ) into its own > jar. There are at least three sipx components that use it now. We > should probably be talking about commons again and assign an issue to > it. > > >> I am a bit uneasy about using DNS to resolve hostnames - even if it's just >> for logging. sipXconfig already knows what the real hostname is. I'll use >> that instead of Hostname class. I do not have time to package that in a >> separate project at the moment but it's coming. > > > Its only just for logging and I request we repackage all of that into > its own jar. We should probably leave well enough alone for the time > being as far as logging goes. It is after all only for debugging > purposes and if it does not pollute your code and lives in its own > jar, maybe we should forge ahead with what already works. > > OK. Let's forge ;-) _______________________________________________ sipx-dev mailing list [email protected] List Archive: http://list.sipfoundry.org/archive/sipx-dev Unsubscribe: http://list.sipfoundry.org/mailman/listinfo/sipx-dev
