jackrabbit-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Felix Meschberger <fmesc...@gmail.com>
Subject Re: Logging of exceptions
Date Thu, 09 Aug 2007 07:41:58 GMT
Hi,

Makes absolute sense, except for throwing the RuntimeException as you
noted in your own follow-up.

And yes, I agree, this would require a big code review. On the other
hand taking the pragmatic approach of fixing those places one after the
other as the issues are encountered is probably the more feasible
approach ...

Regards
Felix

Am Donnerstag, den 09.08.2007, 09:20 +0200 schrieb Christoph Kiehl:
> Felix Meschberger wrote:
> 
> > I agree, that the cause must not be lost. But rather than logging the
> > cause inside loadBundle (in this case), I suggest the
> > hasNonVirtualItemState method should not ignore the exception and log
> > it.
> > 
> > IMHO, if an exception is thrown, the same message should not be logged
> > by the thrower, because the thrower already informs about the situation.
> > Rather the catcher of the exception should handle it by logging or
> > rethrowing.
> > 
> > The problem on the other hand is, that ItemStateException does not
> > always relate a problem: Sometime it is thrown IIRC if an item state is
> > just not availabel and sometimes - as here - it is thrown as a
> > consequence of a possibly real problem.... This distinction can probably
> > not be made by the catcher of the exception ....
> 
> You are absolutely right. I was a bit puzzled because of reasons you 
> mentioned in your last paragraph and thought only the thrower knows 
> whether logging is needed or not.
> 
> But giving it another look I think if the item state is not available a 
> NoSuchItemStateException should be thrown (which is the case in the 
> places I looked at) which is a subclass of ItemStateException. So it 
> would be possible for SharedItemStateManager to distinguish between "not 
> found" and "problem".
> 
> Maybe the following:
> 
> private boolean hasNonVirtualItemState(ItemId id) {
>       [...]
>       } catch (ItemStateException ise) {
>           return false;
>       }
>       [...]
> }
> 
> should be rewritten to:
> 
> private boolean hasNonVirtualItemState(ItemId id) {
>       [...]
>       } catch (NoSuchItemStateException ise) {
>           return false;
>       } catch (ItemStateException ise) {
>           log.error(ise.getMessage(), ise);
>           throw new RuntimeException(ise);
>       }
>       [...]
> }
> 
> And then of course the logging could be removed from this snippet:
> 
> protected synchronized NodePropBundle loadBundle(NodeId id)
>           throws ItemStateException {
>       [...]
>       } catch (Exception e) {
>           String msg = "failed to read bundle: " + id + ": " + e;
>           log.error(msg);
>           throw new ItemStateException(msg, e);
>       } finally {
>       [...]
> }
> 
> to
> 
> protected synchronized NodePropBundle loadBundle(NodeId id)
>           throws ItemStateException {
>       [...]
>       } catch (Exception e) {
>           String msg = "failed to read bundle: " + id + ": ";
>           throw new ItemStateException(msg, e);
>       } finally {
>       [...]
> }
> 
> But I'm afraid that this might become a big change and a lot of code 
> needs to reviewed and checked.
> 
> Cheers,
> Christoph
> 


Mime
View raw message