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 <[EMAIL PROTECTED]> 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
