You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@forrest.apache.org by Ross Gardler <rg...@apache.org> on 2005/12/19 12:50:48 UTC
Re: svn commit: r357622 - /forrest/trunk/main/java/org/apache/forrest/locationmap/LocationMapModule.java
twilliams@apache.org wrote:
> Author: twilliams
> Date: Sun Dec 18 19:11:15 2005
> New Revision: 357622
>
> URL: http://svn.apache.org/viewcvs?rev=357622&view=rev
> Log:
> log null return values, but they aren't errors.
>
> Modified:
> forrest/trunk/main/java/org/apache/forrest/locationmap/LocationMapModule.java
>
> Modified: forrest/trunk/main/java/org/apache/forrest/locationmap/LocationMapModule.java
> URL: http://svn.apache.org/viewcvs/forrest/trunk/main/java/org/apache/forrest/locationmap/LocationMapModule.java?rev=357622&r1=357621&r2=357622&view=diff
> ==============================================================================
> --- forrest/trunk/main/java/org/apache/forrest/locationmap/LocationMapModule.java (original)
> +++ forrest/trunk/main/java/org/apache/forrest/locationmap/LocationMapModule.java Sun Dec 18 19:11:15 2005
> @@ -189,9 +189,10 @@
> }
>
> if (result == null) {
> - String msg = "Locationmap did not return a location for hint " + name;
> - getLogger().error(msg);
> - throw new Exception(msg);
> + if (getLogger().isDebugEnabled()) {
> + String msg = "Locationmap did not return a location for hint " + name;
> + getLogger().debug(msg);
> + }
> }
I made them exceptions because it makes the logs much clearer and, in
fact they are (IMHO) errors. My reasoning is this:
If I have a request for {lm:foo} and there is no match anywhere in the
locationmaps for that value then I cannot possibly evaluate that
location. Therefore Cocoon will be unable to process the request.
Did you miss the fact that I moved the trap to the top level so an
exception is only thrown if *all* mounted locationmaps have been
checked? (previously this trap was done on a per locationmap basis in
which case it would not be an error).
Or is it that I am missing something?
Ross
Re: svn commit: r357622 - /forrest/trunk/main/java/org/apache/forrest/locationmap/LocationMapModule.java
Posted by Ross Gardler <rg...@apache.org>.
Tim Williams wrote:
> On 12/19/05, Ross Gardler <rg...@apache.org> wrote:
>
>>twilliams@apache.org wrote:
>>
>>>Author: twilliams
>>>Date: Sun Dec 18 19:11:15 2005
>>>New Revision: 357622
>>>
>>>URL: http://svn.apache.org/viewcvs?rev=357622&view=rev
>>>Log:
>>>log null return values, but they aren't errors.
>>>
>>>Modified:
>>> forrest/trunk/main/java/org/apache/forrest/locationmap/LocationMapModule.java
>>>
>>>Modified: forrest/trunk/main/java/org/apache/forrest/locationmap/LocationMapModule.java
>>>URL: http://svn.apache.org/viewcvs/forrest/trunk/main/java/org/apache/forrest/locationmap/LocationMapModule.java?rev=357622&r1=357621&r2=357622&view=diff
>>>==============================================================================
>>>--- forrest/trunk/main/java/org/apache/forrest/locationmap/LocationMapModule.java (original)
>>>+++ forrest/trunk/main/java/org/apache/forrest/locationmap/LocationMapModule.java Sun Dec 18 19:11:15 2005
>>>@@ -189,9 +189,10 @@
>>> }
>>>
>>> if (result == null) {
>>>- String msg = "Locationmap did not return a location for hint " + name;
>>>- getLogger().error(msg);
>>>- throw new Exception(msg);
>>>+ if (getLogger().isDebugEnabled()) {
>>>+ String msg = "Locationmap did not return a location for hint " + name;
>>>+ getLogger().debug(msg);
>>>+ }
>>> }
>>
>>
>>I made them exceptions because it makes the logs much clearer and, in
>>fact they are (IMHO) errors. My reasoning is this:
>
>
> I kept the log entry just changed it to debug instead of error.
Which means it only appears, deeply embedded in a huge load of other
messages. Making it very difficult to spot this very common error, but...
> We
> disagree on whether it's an error or not and I'll try to change your
> opinion below...
OK, lets see...
>>If I have a request for {lm:foo} and there is no match anywhere in the
>>locationmaps for that value then I cannot possibly evaluate that
>>location. Therefore Cocoon will be unable to process the request.
>
>
> Sure you can evaluate it and Cocoon can process it. Returning null
> (i.e. saying the location does not exist) *is* evaluating that
> location and the exists selector can process it. Otherwise, we're
> throwing exceptions every time we use the locationmap in an exists
> selector in a sitemap.
> I don't consider it an error in the example
> below when foo does not exist.
> <map:select type="exists">
> <map:when test="{lm:foo}">
> </map:when>
> <map:when test="{lm:bar}">
> </map:when>
> </map:select>
Yes, good point, I had indeed missed that.
I'm not happy with this being a debug as it gets lost and makes
locationmaps difficult to debug when this is an error (i.e. it is not
used in a selector in the sitemap).
How about we make it a warning and change the message?
Ross
Re: svn commit: r357622 - /forrest/trunk/main/java/org/apache/forrest/locationmap/LocationMapModule.java
Posted by Tim Williams <wi...@gmail.com>.
On 12/19/05, Ross Gardler <rg...@apache.org> wrote:
> twilliams@apache.org wrote:
> > Author: twilliams
> > Date: Sun Dec 18 19:11:15 2005
> > New Revision: 357622
> >
> > URL: http://svn.apache.org/viewcvs?rev=357622&view=rev
> > Log:
> > log null return values, but they aren't errors.
> >
> > Modified:
> > forrest/trunk/main/java/org/apache/forrest/locationmap/LocationMapModule.java
> >
> > Modified: forrest/trunk/main/java/org/apache/forrest/locationmap/LocationMapModule.java
> > URL: http://svn.apache.org/viewcvs/forrest/trunk/main/java/org/apache/forrest/locationmap/LocationMapModule.java?rev=357622&r1=357621&r2=357622&view=diff
> > ==============================================================================
> > --- forrest/trunk/main/java/org/apache/forrest/locationmap/LocationMapModule.java (original)
> > +++ forrest/trunk/main/java/org/apache/forrest/locationmap/LocationMapModule.java Sun Dec 18 19:11:15 2005
> > @@ -189,9 +189,10 @@
> > }
> >
> > if (result == null) {
> > - String msg = "Locationmap did not return a location for hint " + name;
> > - getLogger().error(msg);
> > - throw new Exception(msg);
> > + if (getLogger().isDebugEnabled()) {
> > + String msg = "Locationmap did not return a location for hint " + name;
> > + getLogger().debug(msg);
> > + }
> > }
>
>
> I made them exceptions because it makes the logs much clearer and, in
> fact they are (IMHO) errors. My reasoning is this:
I kept the log entry just changed it to debug instead of error. We
disagree on whether it's an error or not and I'll try to change your
opinion below...
> If I have a request for {lm:foo} and there is no match anywhere in the
> locationmaps for that value then I cannot possibly evaluate that
> location. Therefore Cocoon will be unable to process the request.
Sure you can evaluate it and Cocoon can process it. Returning null
(i.e. saying the location does not exist) *is* evaluating that
location and the exists selector can process it. Otherwise, we're
throwing exceptions every time we use the locationmap in an exists
selector in a sitemap. I don't consider it an error in the example
below when foo does not exist.
<map:select type="exists">
<map:when test="{lm:foo}">
</map:when>
<map:when test="{lm:bar}">
</map:when>
</map:select>
Not to mention, throwing exceptions on this makes our error logs
terribly large even though it's a perfectly reasonably approach --
take a look at the error log after building site-author statically.
> Did you miss the fact that I moved the trap to the top level so an
> exception is only thrown if *all* mounted locationmaps have been
> checked? (previously this trap was done on a per locationmap basis in
> which case it would not be an error).
I didn't miss that;)
> Or is it that I am missing something?
I believe this to be the case, but we'll see once we get it worked out.
--tim