cocoon-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Sylvain Wallez <sylv...@apache.org>
Subject Re: svn commit: r312968 - in /cocoon/branches/BRANCH_2_1_X: ./ src/java/org/apache/cocoon/components/source/impl/validity/ src/java/org/apache/cocoon/i18n/ src/java/org/apache/cocoon/transformation/ src/webapp/WEB-INF/ src/webapp/samples/i18n/translations/
Date Sun, 23 Oct 2005 09:21:48 GMT
Vadim Gritsenko wrote:
> Sylvain Wallez wrote:
>> Why such a complicated exception handling?
>
> Same as before.
>
>
>> 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.

That can be changed though: BlobSource can be fixed by querying the DB 
in exists(), and SitemapSource can tell if a pipeline was built.

>   * 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? 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...

> 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.

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.

Sylvain

-- 
Sylvain Wallez                        Anyware Technologies
http://people.apache.org/~sylvain     http://www.anyware-tech.com
Apache Software Foundation Member     Research & Technology Director


Mime
View raw message