qpid-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Rupert Smith" <rupertlssm...@googlemail.com>
Subject Re: Java Broker Exception Handling and Logging.
Date Fri, 18 May 2007 08:59:19 GMT
Arnaud,

I'm glad we seem to be in general agreement. Don't worry too much about
eliminating spurios logging in your code, I will have to go through all of
the code with my todo list anyway, so will spot any double loggings. I will
probably just downgrade log.error on exception creation to log.debug, and
check that the exception will actually make it to a handling point where its
logged (if it really is an error).

Also, if I eliminate any of the many exception constructors, I will change
calling code to call just one constructor, but leave the others in marked as
deprecated for a while, so as not to play nice with code other people are
still working on.

I'm not going to add or delete any exceptions, perhpas alter the class
hierarchy a little in a few places, but really existing code and stuff you
are working on at the moment should be able to use the existing exceptions
without being impacted.

Rupert

On 18/05/07, Arnaud Simon <asimon@redhat.com> wrote:
>
> Rupert,
>
> > We know there are some things about the existing exception handling that
> are
> > not right. In particular when the broker encounters unrecoverable
> > conditions, it will often go limping on in an unknown state, when really
> it
> > should simply terminate the JVM.
>
> +1
>
> > There is also some dodgy logging code, spread throughout the codebase,
> that
> > causes exceptions to be logged mutliple times, in constructors, on
> creation,
> > in catch and rethrow scenarios and so on; I've mailed about this before.
> I
> > had to write a document explaining to sys admins what to do when they
> see
> > various log messages, so as a side to this I have compiled a list of
> todo's
> > to eliminate the dodgy logging. Logging will be done where exceptions
> are
> > handled and never where they are created (at least logging above the
> debug
> > level, which is for devs only and can occur anywhere we like).
>
> I agree with that, I'll be careful now as I may have logged my
> exceptions when throwing them.
>
> > Some thoughts:
> >
> ....
> > So, I think its correct that you have made message store throw a checked
> > QueueDoesntExistException, if a method handler looks up a non-existant
> > queue, it might translate that into a 404. Other code that calls that
> > method, for example during start-up and configuration, will deal with
> that
> > exception in its own way, and not as a 404, since it doesn't go through
> the
> > protocol.
>
> I agree 100% with what you are saying. I have introduced
> InternalErrorException because it should be translated into an error
> code. This is part of the dtx API. But however for consistency and for
> avoiding this exception being abused we should use a Runtime one.
>
> > One thing I'm keen on is making exceptions only have one constructor,
> and
> > passing in optional arguments as nulls. Its not a huge deal, its just
> that I
> > like to Javadoc stuff and its a pain to write three lots of Javadoc and
> > contructors for such trivial things as exceptions. I never understand
> why it
> > is that exceptions in particular seem to end up with huge numbers of
> > different constructors for every permutation of options. AMQException
> has 4
> > and thats after I got rid of another 3! On that note, I think a bit of
> > javadoc on all exception classes explaining the conditions under which
> the
> > exception may be thrown, is extremely usefull.
>
> +1
>
> As I don't want to walk on your toes we should coordinate. One way of
> doing it is for you to define the new hierarchy. I can then go through
> my code for:
> 1) removing useless logging statements
> 2) use your Exception hierarchy
>
> What do you think? BTW I have already written a JDBC store that uses the
> same exception strategy than before. I suggest that we check it in as it
> is. I'll update the code later based on your exception hierarchy.
>
> Arnaud
>
>
>
>
>

Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message