Sylvain Wallez wrote:
Vadim Gritsenko wrote:
Sylvain Wallez wrote:
I see several problems here:
- SourceNotFoundException is a nominal case (BTW why not using
Source.exists()?) and should not log an exception
* Source.exists() will not work here:
SitemapSource:
/**
* Returns true always.
* @see org.apache.excalibur.source.Source#exists()
*/
public boolean exists() {
return true;
}
BlobSource:
/**
* @see org.apache.excalibur.source.Source#exists()
*/
public boolean exists() {
// FIXME
return true;
}
That's why SourceNotFoundException is a nominal case: some source
implementations cannot say beforehand if they exist or not.
So? I don't see your point.
That can be changed though: BlobSource can be fixed by querying the DB
in exists(), and SitemapSource can tell if a pipeline was built.
Agreed.
* Exception is *not* logged (in the INFO level, that is):
+ } else if (getLogger().isInfoEnabled()) {
+ getLogger().info("Bundle <" + sourceURI + "> not
loaded: Source URI not found");
+ }
* Exception *must* be logged if Category is in DEBUG
level - exactly what you would expect if you are trying to
debug i18n component. Anything else is counter productive
and counter intuitive (what you would do if you need to see
those exeptions? Set a breakpoint? Patch implementation?
Roll your own implementation?)
- Bad formed XML files and other serious exceptions are semi-silently
ignored. By semi-silently, I mean they're just logged and don't
bubble up higher in the call stack, thus giving the false impression
that the system works.
Such exceptions must not bubble up upstream: if exception is let
through, your whole site goes down simply due to single bug in i18n
catalogue. With existing exception handling, i18n (and your whole
site) continues functioning with older version of the catalogue, but
reports an error into the log file (that's what you've got monitoring
for). That's the i18n behaviour as it was originally designed. See
"Keep existing loaded values" comment.
Ok. So you mean that i18n allows broken message files to exist?
Exactly.
This is
contradictory with *all* other hot-reload behaviours in Cocoon: if an
XSLT, a template or sitemap are modified and are malformed, an error is
raised and bubbles up (yes, potentially breaking the whole system). We
don't use the cached version of the file if reload fails.
That's why I find the way i18n handles this very strange. Or does it
mean message dictionaries are not considered on an equal stand with
other application files, and are allowed to be buggy and changed live on
the production server without testing beforehand? This really doesn't
sound good to me...
I guess it takes some getting used to it.
OTOH, if source was deleted, it wipes out old values, as that is what
you are expecting it to do in case you want to delete existing catalogue.
Right.
I will fix the 1st point (it scares my users to see all this in the
logs), and would really understand the rationale for the second.
If "fix" means always drop original exception, -1: Original exception
must be logged somewhere in case DEBUG is turned on (tell your users
to use INFO or WARN level - DEBUG is for *debugging*).
Problem is that SourceNotFoundException is a *nominal* case indicating
that the source doesn't exist, and which i18n allows since it provides
hierarchical lookup of message files. Logging the exception even in
debug mode gives the false impression that something goes wrong when
this isn't the case.
Then perfect solution is to have a message that reads...
DEBUG (2005-10-17) 15:35.59:744 [i18n] (/foo/bar)
Unknown-Thread/XMLResourceBundleFactory: This is a DEBUG message, you dummy!
SourceNotFoundException happened while resolving a bundle. This bundle probably
does not exist, and that is Ok, so DO NOT WORRY, BUT SET YOUR FREAKING CONFIG TO
INFO! If you are debugging i18n behavior or Cocoon treeprocessor and pipelines,
though, following stacktrace might be useful:
...
Or some such - tweak the wording to your liking...
More general note - ignored exceptions is the single most frustrating experience
you can have with Cocoon in particular and Java in general. Hence I'm proponent
of having the ability to see exception if so desired.
SNFE is used here as a substitute for source.exist(), probably because
two implementations don't have a proper implementation for it. Better
fix the implementations or log the exception only if source.exists()
returns true rather than fill the logs with meaningless exceptions.
Won't argue with that. OTOH, there might be broken sources out there where even
if source.exists() it can still throw SNFE.
You also have to take into an account a situation where source WAS existing at
the moment of .exists(), but was removed before you tried to .getInputStream()
it. So, SNFE handling still has to be present.
Vadim
