cocoon-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Vadim Gritsenko <va...@reverycodes.com>
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 Mon, 24 Oct 2005 13:21:35 GMT
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

Mime
View raw message