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 ).
>
> 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.
>
> 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.
>
> 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.
> D.
>
> _______________________________________________
> sipx-dev mailing list
> [email protected]
> List Archive: http://list.sipfoundry.org/archive/sipx-dev
> Unsubscribe: http://list.sipfoundry.org/mailman/listinfo/sipx-dev
>
--
M. Ranganathan
_______________________________________________
sipx-dev mailing list
[email protected]
List Archive: http://list.sipfoundry.org/archive/sipx-dev
Unsubscribe: http://list.sipfoundry.org/mailman/listinfo/sipx-dev
