Sounds reasonable to me. Thanks for tackling this, Claude! On Sun, Aug 30, 2015 at 9:07 AM, Claude Brisson <cla...@renegat.net> wrote:
> Some remarks after a little investigation. > > 1) Using the ServletContext logger doesn't seem to be a widespread > practice among slf4j folks. There is a slf4j-servletcontext implementation > on github, but it is not referenced on the official slf4j site. Moreover, I > tested it, and it does not support deserialization (I had some doubts about > it, because deserialization may happen before ServletContext > initialization), so it cannot be used as is. Plus, it needs to be declared > as context's listener in the deployment file. > > It's possible to solve those two problems (alas not from our own codebase): > - The logger object can be a proxy to a transcient implementation that > waits for the servlet context to be initialized. > - Since J2EE API v3.0, there is no need to declare the listener in the > deployment descriptor, as it can be done using Java annotations, which > would fit slf4j 'jar'-only binding strategy far more than the current state > of the project. > > But not using the ServletContext logger is not necessarily a bad idea. Of > course, "it's here", so why not use it, but: > - In practice, container logger and webapp logger serve rather different > purposes, as container logs won't be active but at initialization or > destruction. > - They're not formatted the same way. > - It's not so cumbersome to watch several log files at once (I mean, on a > real O.S. with a proper command line, of course) when you need to see both > at the same time. > - It's much easier to configure a per-webapp logging (and consequently to > ease webapp deployment). > - Gathering logs is always doable at the container level, which can also > use slf4j (or one of its many bridges), bypassing ServletContext (which is > just a proxy). > > Anyhow, even it's a little drawback for some people, it can not weight as > much in the balance as being freed of all logging > > 2) We can -and should- keep our logger injection mechanism, since it not > only helps separating different instances logs, but also permits unifying > log line prefixes. No more need to define the logger in > velocity.properties, by default it will be a static logger named > "Velocity". The only constraint when trying to separate instances logs is > to explicitly give different names to each logger (since the underlying > LoggerFactory mechanism remains static). Not a big deal. > > Without any other objection, I'll start this drastic simplification very > soon. We don't even need to keep the o.a.v.runtime.log package. And four > sub-modules can disappear... > > Claude > > > On 21/08/2015 17:25, Nathan Bubna wrote: > >> I like simple. :) >> >> On Fri, Aug 21, 2015 at 2:18 AM, Claude Brisson<cla...@renegat.net> >> wrote: >> >> It looks like that with slf4j, we could keep loggers injection even in >>> session tools, so no need to go static: >>> >>> "As of SLF4J version 1.5.3, logger instances survive serialization. Thus, >>> serialization of the host class no longer requires any special action, >>> even >>> when loggers are declared as instance variables. In previous versions, >>> logger instances needed to be declared as transient in the host class." >>> (http://www.slf4j.org/faq.html#declared_static ) >>> >>> >>> So it looks like the perfect solution, and will simplify many things. >>> >>> Claude >>> >>> >>> >>> On 20/08/2015 21:32, Sergiu Dumitriu wrote: >>> >>> A recent discussion was proposing to switch to slf4j as the façade. >>>> On Aug 20, 2015 3:12 PM, "Claude Brisson"<cla...@renegat.net> wrote: >>>> >>>> Thanks for the review. >>>> >>>>> Yes, we should be using Velocity logging facade, but I just don't see >>>>> for >>>>> now how we should do this while preserving tools serialization. >>>>> >>>>> For now I just mimicked a behavior present in some deprecated classes, >>>>> re-introducing the dependency of velocity-tools-view on >>>>> commons-logging. >>>>> >>>>> I agree that we should try to do better, but if a logger cannot be >>>>> statically reached from a session tool, and if it cannot be passed at >>>>> initialization because of serialization, there's a problem... >>>>> >>>>> Let say for instance that we use transient weak references on >>>>> non-serializable Velocity loggers, fed up at initialization time. Then >>>>> the >>>>> tool could detect it has lost its logger at deserialization, and ask a >>>>> new >>>>> one, but to whom? It's a bad idea either to keep a static reference on >>>>> the >>>>> servlet context. >>>>> >>>>> I'm open to suggestions... >>>>> >>>>> >>>>> Claude >>>>> >>>>> >>>>> On 20/08/2015 20:45, Sergiu Dumitriu wrote: >>>>> >>>>> Isn't the logging framework supposed to be pluggable, meaning that all >>>>> >>>>>> calls go through org.apache.velocity.runtime.log.Log which is then >>>>>> passed to the actual logging library in use, be it log4j, slf4j, jcl >>>>>> or >>>>>> commons-logging? >>>>>> >>>>>> On 08/20/2015 01:16 PM,cbris...@apache.org wrote: >>>>>> >>>>>> Author: cbrisson >>>>>> >>>>>>> Date: Thu Aug 20 17:16:02 2015 >>>>>>> New Revision: 1696822 >>>>>>> >>>>>>> URL:http://svn.apache.org/r1696822 >>>>>>> Log: >>>>>>> J2EE containers session serialization implies static logging from >>>>>>> session scoped tools >>>>>>> >>>>>>> Modified: >>>>>>> velocity/tools/trunk/src/site/xdoc/changes.xml >>>>>>> >>>>>>> >>>>>>> >>>>>>> velocity/tools/trunk/velocity-tools-struts/src/main/java/org/apache/velocity/tools/struts/TilesTool.java >>>>>>> >>>>>>> >>>>>>> >>>>>>> velocity/tools/trunk/velocity-tools-view/src/main/java/org/apache/velocity/tools/view/AbstractSearchTool.java >>>>>>> >>>>>>> >>>>>>> >>>>>>> velocity/tools/trunk/velocity-tools-view/src/main/java/org/apache/velocity/tools/view/BrowserTool.java >>>>>>> >>>>>>> >>>>>>> >>>>>>> velocity/tools/trunk/velocity-tools-view/src/main/java/org/apache/velocity/tools/view/ImportSupport.java >>>>>>> >>>>>>> >>>>>>> >>>>>>> velocity/tools/trunk/velocity-tools-view/src/main/java/org/apache/velocity/tools/view/tools/ImportTool.java >>>>>>> >>>>>>> Modified: velocity/tools/trunk/src/site/xdoc/changes.xml >>>>>>> URL: >>>>>>> >>>>>>> >>>>>>> http://svn.apache.org/viewvc/velocity/tools/trunk/src/site/xdoc/changes.xml?rev=1696822&r1=1696821&r2=1696822&view=diff >>>>>>> >>>>>>> >>>>>>> >>>>>>> ============================================================================== >>>>>>> --- velocity/tools/trunk/src/site/xdoc/changes.xml (original) >>>>>>> +++ velocity/tools/trunk/src/site/xdoc/changes.xml Thu Aug 20 >>>>>>> 17:16:02 >>>>>>> 2015 >>>>>>> @@ -35,6 +35,7 @@ >>>>>>> <li>Reflect Velocity Engine dependency shading of >>>>>>> commons-lang >>>>>>> and commons-collections (cb)</li> >>>>>>> <li>Removed deprecated class >>>>>>> org.apache.velocity.tools.generic.log.LogSystemCommonsLog (cb)</li> >>>>>>> <li>MathTool: fixed result type calculations and added >>>>>>> bitwise >>>>>>> operations (cb)</li> >>>>>>> + <li>have session scoped tools use a static logger to fix >>>>>>> J2EE >>>>>>> containers session serialization (cb)</li> >>>>>>> </ul> >>>>>>> </subsection> >>>>>>> >>>>>>> Modified: >>>>>>> >>>>>>> >>>>>>> velocity/tools/trunk/velocity-tools-struts/src/main/java/org/apache/velocity/tools/struts/TilesTool.java >>>>>>> URL: >>>>>>> >>>>>>> >>>>>>> http://svn.apache.org/viewvc/velocity/tools/trunk/velocity-tools-struts/src/main/java/org/apache/velocity/tools/struts/TilesTool.java?rev=1696822&r1=1696821&r2=1696822&view=diff >>>>>>> >>>>>>> >>>>>>> >>>>>>> ============================================================================== >>>>>>> --- >>>>>>> >>>>>>> >>>>>>> velocity/tools/trunk/velocity-tools-struts/src/main/java/org/apache/velocity/tools/struts/TilesTool.java >>>>>>> (original) >>>>>>> +++ >>>>>>> >>>>>>> >>>>>>> velocity/tools/trunk/velocity-tools-struts/src/main/java/org/apache/velocity/tools/struts/TilesTool.java >>>>>>> Thu Aug 20 17:16:02 2015 >>>>>>> @@ -101,7 +101,6 @@ public class TilesTool extends ImportSup >>>>>>> setRequest(ctx.getRequest()); >>>>>>> setResponse(ctx.getResponse()); >>>>>>> setServletContext(ctx.getServletContext()); >>>>>>> - setLog(ctx.getVelocityEngine().getLog()); >>>>>>> } >>>>>>> } >>>>>>> >>>>>>> Modified: >>>>>>> >>>>>>> >>>>>>> velocity/tools/trunk/velocity-tools-view/src/main/java/org/apache/velocity/tools/view/AbstractSearchTool.java >>>>>>> URL: >>>>>>> >>>>>>> >>>>>>> http://svn.apache.org/viewvc/velocity/tools/trunk/velocity-tools-view/src/main/java/org/apache/velocity/tools/view/AbstractSearchTool.java?rev=1696822&r1=1696821&r2=1696822&view=diff >>>>>>> >>>>>>> >>>>>>> >>>>>>> ============================================================================== >>>>>>> --- >>>>>>> >>>>>>> >>>>>>> velocity/tools/trunk/velocity-tools-view/src/main/java/org/apache/velocity/tools/view/AbstractSearchTool.java >>>>>>> (original) >>>>>>> +++ >>>>>>> >>>>>>> >>>>>>> velocity/tools/trunk/velocity-tools-view/src/main/java/org/apache/velocity/tools/view/AbstractSearchTool.java >>>>>>> Thu Aug 20 17:16:02 2015 >>>>>>> @@ -22,7 +22,8 @@ package org.apache.velocity.tools.view; >>>>>>> import java.util.Collections; >>>>>>> import java.util.List; >>>>>>> import javax.servlet.http.HttpServletRequest; >>>>>>> -import org.apache.velocity.runtime.log.Log; >>>>>>> +import org.apache.commons.logging.Log; >>>>>>> +import org.apache.commons.logging.LogFactory; >>>>>>> import org.apache.velocity.tools.Scope; >>>>>>> import org.apache.velocity.tools.config.DefaultKey; >>>>>>> import org.apache.velocity.tools.config.InvalidScope; >>>>>>> @@ -129,19 +130,11 @@ public abstract class AbstractSearchTool >>>>>>> protected static final String STORED_RESULTS_KEY = >>>>>>> StoredResults.class.getName(); >>>>>>> - protected Log LOG; >>>>>>> + protected static final Log LOG = >>>>>>> LogFactory.getLog(AbstractSearchTool.class); >>>>>>> + >>>>>>> private String criteriaKey = DEFAULT_CRITERIA_KEY; >>>>>>> private Object criteria; >>>>>>> - public void setLog(Log log) >>>>>>> - { >>>>>>> - if (log == null) >>>>>>> - { >>>>>>> - throw new NullPointerException("log should not be set to >>>>>>> null"); >>>>>>> - } >>>>>>> - this.LOG = log; >>>>>>> - } >>>>>>> - >>>>>>> /** >>>>>>> * Sets the criteria *if* it is set in the request >>>>>>> parameters. >>>>>>> */ >>>>>>> >>>>>>> Modified: >>>>>>> >>>>>>> >>>>>>> velocity/tools/trunk/velocity-tools-view/src/main/java/org/apache/velocity/tools/view/BrowserTool.java >>>>>>> URL: >>>>>>> >>>>>>> >>>>>>> http://svn.apache.org/viewvc/velocity/tools/trunk/velocity-tools-view/src/main/java/org/apache/velocity/tools/view/BrowserTool.java?rev=1696822&r1=1696821&r2=1696822&view=diff >>>>>>> >>>>>>> >>>>>>> >>>>>>> ============================================================================== >>>>>>> --- >>>>>>> >>>>>>> >>>>>>> velocity/tools/trunk/velocity-tools-view/src/main/java/org/apache/velocity/tools/view/BrowserTool.java >>>>>>> (original) >>>>>>> +++ >>>>>>> >>>>>>> >>>>>>> velocity/tools/trunk/velocity-tools-view/src/main/java/org/apache/velocity/tools/view/BrowserTool.java >>>>>>> Thu Aug 20 17:16:02 2015 >>>>>>> @@ -24,7 +24,8 @@ import java.util.regex.Pattern; >>>>>>> import java.util.regex.PatternSyntaxException; >>>>>>> import java.util.*; >>>>>>> import javax.servlet.http.HttpServletRequest; >>>>>>> -import org.apache.velocity.runtime.log.Log; >>>>>>> +import org.apache.commons.logging.Log; >>>>>>> +import org.apache.commons.logging.LogFactory; >>>>>>> import org.apache.velocity.tools.Scope; >>>>>>> import org.apache.velocity.tools.ConversionUtils; >>>>>>> import org.apache.velocity.tools.generic.FormatConfig; >>>>>>> @@ -73,8 +74,8 @@ import org.apache.velocity.tools.config. >>>>>>> public class BrowserTool extends FormatConfig implements >>>>>>> java.io.Serializable >>>>>>> { >>>>>>> private static final long serialVersionUID = >>>>>>> 1734529350532353339L; >>>>>>> - >>>>>>> - protected Log LOG; >>>>>>> + >>>>>>> + protected static final Log LOG = >>>>>>> LogFactory.getLog(BrowserTool.class); >>>>>>> /* User-Agent header variables */ >>>>>>> private String userAgent = null; >>>>>>> @@ -187,19 +188,6 @@ public class BrowserTool extends FormatC >>>>>>> } >>>>>>> /** >>>>>>> - * Set log. >>>>>>> - */ >>>>>>> - public void setLog(Log log) >>>>>>> - { >>>>>>> - if (log == null) >>>>>>> - { >>>>>>> - throw new NullPointerException("log should not be set to >>>>>>> null"); >>>>>>> - } >>>>>>> - this.LOG = log; >>>>>>> - } >>>>>>> - >>>>>>> - >>>>>>> - /** >>>>>>> * Sets the User-Agent string to be parsed for info. If >>>>>>> null, >>>>>>> the >>>>>>> string >>>>>>> * will be empty and everything will return false or null. >>>>>>> Otherwise, >>>>>>> * it will set the whole string to lower case before storing >>>>>>> to >>>>>>> simplify >>>>>>> >>>>>>> Modified: >>>>>>> >>>>>>> >>>>>>> velocity/tools/trunk/velocity-tools-view/src/main/java/org/apache/velocity/tools/view/ImportSupport.java >>>>>>> URL: >>>>>>> >>>>>>> >>>>>>> http://svn.apache.org/viewvc/velocity/tools/trunk/velocity-tools-view/src/main/java/org/apache/velocity/tools/view/ImportSupport.java?rev=1696822&r1=1696821&r2=1696822&view=diff >>>>>>> >>>>>>> >>>>>>> >>>>>>> ============================================================================== >>>>>>> --- >>>>>>> >>>>>>> >>>>>>> velocity/tools/trunk/velocity-tools-view/src/main/java/org/apache/velocity/tools/view/ImportSupport.java >>>>>>> (original) >>>>>>> +++ >>>>>>> >>>>>>> >>>>>>> velocity/tools/trunk/velocity-tools-view/src/main/java/org/apache/velocity/tools/view/ImportSupport.java >>>>>>> Thu Aug 20 17:16:02 2015 >>>>>>> @@ -39,12 +39,17 @@ import javax.servlet.ServletOutputStream >>>>>>> import javax.servlet.http.HttpServletRequest; >>>>>>> import javax.servlet.http.HttpServletResponse; >>>>>>> import javax.servlet.http.HttpServletResponseWrapper; >>>>>>> -import org.apache.velocity.runtime.log.Log; >>>>>>> +import org.apache.commons.logging.Log; >>>>>>> +import org.apache.commons.logging.LogFactory; >>>>>>> + >>>>>>> /** >>>>>>> * <p>Provides methods to import arbitrary local or remote >>>>>>> resources >>>>>>> as strings.</p> >>>>>>> * <p>Based on ImportSupport from the JSTL taglib by Shawn >>>>>>> Bayern</p> >>>>>>> * >>>>>>> + * <p>If you wish to take profit of your J2EE container session >>>>>>> serialization, >>>>>>> + * inheriting tools should always be in the request scope.</p> >>>>>>> + * >>>>>>> * @author <a href="mailto:mari...@centrum.is";>Marino A. >>>>>>> Jonsson</a> >>>>>>> * @since VelocityTools 2.0 >>>>>>> * @version $Revision$ $Date$ >>>>>>> @@ -57,7 +62,8 @@ public abstract class ImportSupport >>>>>>> /** Default character encoding for response. */ >>>>>>> protected static final String DEFAULT_ENCODING = >>>>>>> "ISO-8859-1"; >>>>>>> - protected Log LOG; >>>>>>> + protected static final Log LOG = >>>>>>> LogFactory.getLog(ImportSupport.class); >>>>>>> + >>>>>>> protected ServletContext application; >>>>>>> protected HttpServletRequest request; >>>>>>> protected HttpServletResponse response; >>>>>>> @@ -65,15 +71,6 @@ public abstract class ImportSupport >>>>>>> // --------------------------------------- Setup Methods >>>>>>> ------------- >>>>>>> - public void setLog(Log log) >>>>>>> - { >>>>>>> - if (log == null) >>>>>>> - { >>>>>>> - throw new NullPointerException("log should not be set to >>>>>>> null"); >>>>>>> - } >>>>>>> - this.LOG = log; >>>>>>> - } >>>>>>> - >>>>>>> /** >>>>>>> * Sets the current {@link HttpServletRequest}. This is >>>>>>> required >>>>>>> * for this tool to operate and will throw a >>>>>>> NullPointerException >>>>>>> >>>>>>> Modified: >>>>>>> >>>>>>> >>>>>>> velocity/tools/trunk/velocity-tools-view/src/main/java/org/apache/velocity/tools/view/tools/ImportTool.java >>>>>>> URL: >>>>>>> >>>>>>> >>>>>>> http://svn.apache.org/viewvc/velocity/tools/trunk/velocity-tools-view/src/main/java/org/apache/velocity/tools/view/tools/ImportTool.java?rev=1696822&r1=1696821&r2=1696822&view=diff >>>>>>> >>>>>>> >>>>>>> >>>>>>> ============================================================================== >>>>>>> --- >>>>>>> >>>>>>> >>>>>>> velocity/tools/trunk/velocity-tools-view/src/main/java/org/apache/velocity/tools/view/tools/ImportTool.java >>>>>>> (original) >>>>>>> +++ >>>>>>> >>>>>>> >>>>>>> velocity/tools/trunk/velocity-tools-view/src/main/java/org/apache/velocity/tools/view/tools/ImportTool.java >>>>>>> Thu Aug 20 17:16:02 2015 >>>>>>> @@ -36,7 +36,6 @@ public class ImportTool extends org.apac >>>>>>> setRequest(ctx.getRequest()); >>>>>>> setResponse(ctx.getResponse()); >>>>>>> setServletContext(ctx.getServletContext()); >>>>>>> - setLog(ctx.getVelocityEngine().getLog()); >>>>>>> } >>>>>>> } >>>>>>> } >>>>>>> >>>>>>> >>>>>>> >>>>>>> --------------------------------------------------------------------- >>>>>>> >>>>>> To unsubscribe, e-mail:dev-unsubscr...@velocity.apache.org >>>>> For additional commands, e-mail:dev-h...@velocity.apache.org >>>>> >>>>> >>>>> >>>>> --------------------------------------------------------------------- >>> To unsubscribe, e-mail:dev-unsubscr...@velocity.apache.org >>> For additional commands, e-mail:dev-h...@velocity.apache.org >>> >>> >>> > > --------------------------------------------------------------------- > To unsubscribe, e-mail: dev-unsubscr...@velocity.apache.org > For additional commands, e-mail: dev-h...@velocity.apache.org > >
