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
