Hi Omair,
----- Omair Majid <omajid at redhat.com> wrote:
> Hi Jiri,
>
> I am mostly interested in this patch as a user (especially if some else
> has agreed to a full review of this giant patch). But it caught my
> interest so I have a few questions.
>
> First, please don't abuse 'refactoring'. It's defined here:
> http://refactoring.com/. This patch is a change (probably a good one);
> it's not (just) a refactor.
>
> On 09/10/2013 04:19 PM, Jiri Vanek wrote:
> > This patch is introducing bottleneck for all stdout/err and
> > printstacktrace.
>
> I thought bottlenecks were bad? ;)
>
> > By this it is first part of
> > http://icedtea.classpath.org/wiki/IcedTea-Web#IcedTea-Web_1.5 "introduce
> > new logging and think about abrt connection"
>
> Okay, but that's not explaining things any better :)
>
> > Inspiration:
> > - get rid of repeated if JNLPRuntime.isDebug
>
> I am sure it's not relevant here, but OpenJDK switched from:
>
> log(foo)
>
> to
>
> if (debug) {
> log(foo)
> }
>
> in a couple of places due to performance reasons. Just something to keep
> in mind.
>
> > - log properly into syslog (comming as next patch(*)(**) after this one
> > s in)
>
> This sounds (at once) really cool and really scary.
>
> Can you elaborate on exactly what you mean by syslog? Is it the syslog
> protocol as defined in RFC 3164? What if a syslog daemon is not running?
Yeah we thought about this one.
>
> Can you describe the motivation for logging into syslog? Is this
> somethings lots of other user programs do?
The main problem with the current solution is that most users don't know
how to enable debug output and also how to run ITW (+browser) from the console
to see error output. Syslog might be better and I think it's now the standard
way how applications might log its messages.
>
> > - revive the old logger which logs into ~/..../logs to log more then
> > few start up exception (if enabled)
>
> Are you saying the old logger is broken? Can you elaborate?
>
> > - maybe something more, I got very asleep :)
>
> You mean to say, you are not working 24/7 any more? That's not acceptable :P
Yeah, Jiri really should improve his ability to work 24/7! :)
>
> > The only real code is in extractedLogging-bottleNeck. The syslog
> > writer(*) is not yet implemented and will be inside method write, which
> > is now just dummy
>
> Have you looked at existing implementations? Maybe we can reuse something?
We can, I'll inform you when simple demo will be created.
>
> > (**) The best way how to integrate the syslogs writer is still under
> > research (tcp?, exec?)
>
> External-library?
>
> > The extractedLogging-refactoring is extremely long, And I do not expect
> > somebody to read it whole.
> > It is moreover just all the same:
> >
> > - if (JNLPRuntime.isDebug())
> > - System.err.println("UNIQUEKEY=" + this.uniqueKey);
> > + ItwLogger.getLogger().logErrorStream("UNIQUEKEY=" +
> > this.uniqueKey);
>
> Just to be clear, logging is not the same as messages displayed on
> stderr. There are probably some messages that we still want to show to
> the user on stderr (and log too, maybe).
>
> > or
> >
> > - if (JNLPRuntime.isDebug())
> > - ex.printStackTrace();
> > + ItwLogger.getLogger().log(ex);
> >
> > or its variants without if (JNLPRuntime.isDebug()) and so adding "true"
> > parameter which force logging always:
> >
> > - System.err.println("UNIQUEKEY=" + this.uniqueKey);
> > + ItwLogger.getLogger().logErrorStream(true, "UNIQUEKEY=" +
> > this.uniqueKey);
> >
> > or
> >
> > - ex.printStackTrace();
> > + ItwLogger.getLogger().log(true, ex);
> >
> > And hope I did not miss something or not changed if debug/if not debug
> > (/me have to try reprodcuers with/without patch)
>
> I am probably missing something, but why isn't ItwLogger a
> java.util.logging.Logger?
>
> Please add javadocs to indicate when to use which methods. Also, please
> add unit tests for (at least) the ItwLogger class.
>
> > The only real changes are minor and are in
> > /tests/netx/unit/net/sourceforge/jnlp/DefaultLaunchHandlerTest.java
>
> s/baos1/outputStream and s/baos2/errorStream/
>
> > and removed duplicated code arround exceptionToString.
> > netx/net/sourceforge/jnlp/ParseException.java (removed redundant code
> > 1.3 compatibility..hehe)
>
> A separate patch, please!
>
> Cheers,
> Omair
>
> --
> PGP Key: 66484681 (http://pgp.mit.edu/)
> Fingerprint = F072 555B 0A17 3957 4E95 0056 F286 F14F 6648 4681