jackrabbit-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Christoph Kiehl ...@sulu3000.de>
Subject Re: Logging of exceptions
Date Thu, 09 Aug 2007 07:20:29 GMT
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