Return-Path: Delivered-To: apmail-jakarta-tomcat-dev-archive@apache.org Received: (qmail 73546 invoked from network); 6 Aug 2002 20:29:11 -0000 Received: from unknown (HELO nagoya.betaversion.org) (192.18.49.131) by daedalus.apache.org with SMTP; 6 Aug 2002 20:29:11 -0000 Received: (qmail 10972 invoked by uid 97); 6 Aug 2002 20:29:27 -0000 Delivered-To: qmlist-jakarta-archive-tomcat-dev@jakarta.apache.org Received: (qmail 10956 invoked by uid 97); 6 Aug 2002 20:29:27 -0000 Mailing-List: contact tomcat-dev-help@jakarta.apache.org; run by ezmlm Precedence: bulk List-Unsubscribe: List-Subscribe: List-Help: List-Post: List-Id: "Tomcat Developers List" Reply-To: "Tomcat Developers List" Delivered-To: mailing list tomcat-dev@jakarta.apache.org Received: (qmail 10939 invoked by uid 98); 6 Aug 2002 20:29:27 -0000 X-Antivirus: nagoya (v4198 created Apr 24 2002) Date: Tue, 6 Aug 2002 13:28:55 -0700 (PDT) From: "Craig R. McClanahan" To: Tomcat Developers List Subject: Re: [5] commons-logging In-Reply-To: <1028665194.12735.76.camel@dhcp-70-230> Message-ID: <20020806132331.Y71348-100000@icarus.apache.org> MIME-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII X-Spam-Rating: localhost 1.6.2 0/1000/N X-Spam-Rating: daedalus.apache.org 1.6.2 0/1000/N X-Spam-Rating: daedalus.apache.org 1.6.2 0/1000/N On 6 Aug 2002, Bob Herrmann wrote: > Date: 06 Aug 2002 16:19:54 -0400 > From: Bob Herrmann > Reply-To: Tomcat Developers List > To: Tomcat Developers List > Subject: [5] commons-logging > > > "Just for fun", I updated ApplicationDispatcher.java to use > commons-logging. It may not be representative in some ways (it defines > its own debug level), but it was interesting to see what the changes > look like. > > Does it look like I changed it correctly? (especially the way I chose > to initialize a private field "Log" in the constructor.) > A couple of suggestions. * Conventions for naming Log instances will undoubtedly need to be discussed and agreed to. A common convention in the commons packages is to use the fully qualified classname, but that would mean that all the ApplicationDispatcher instances for all webapps would share a common log. Your approach of using the context path is a good start, but isn't necessarily unique if you have multiple virtual hosts. * We should lose the old debug setting when switching to use commons-logging. It's purpose was to accomplish something like what Log4J and JDK 1.4 let you do (configure the log detail level for a particular module), so they become redundant now. So, things like "if (debug > 1 && log.isDebugEnabled())" would just become "if (log.isDebugEnabled())". * At some point, some common conventions about what logging levels to use for what sort of thing across Tomcat will be appropriate. A "best practices" document was checked in to commons-logging as a starting point for this -- I don't necessarily agree with every word but it's a good starting point for thinking. > Cheers, > -bob > Craig > Index: catalina/src/share/org/apache/catalina/core/ApplicationDispatcher.java > =================================================================== > RCS file: /home/cvspublic/jakarta-tomcat-catalina/catalina/src/share/org/apache/catalina/core/ApplicationDispatcher.java,v > retrieving revision 1.1.1.1 > diff -u -r1.1.1.1 ApplicationDispatcher.java > --- catalina/src/share/org/apache/catalina/core/ApplicationDispatcher.java 18 Jul 2002 16:48:06 -0000 1.1.1.1 > +++ catalina/src/share/org/apache/catalina/core/ApplicationDispatcher.java 6 Aug 2002 20:09:50 -0000 > @@ -88,7 +88,6 @@ > import org.apache.catalina.HttpRequest; > import org.apache.catalina.HttpResponse; > import org.apache.catalina.InstanceEvent; > -import org.apache.catalina.Logger; > import org.apache.catalina.Request; > import org.apache.catalina.Response; > import org.apache.catalina.Wrapper; > @@ -97,7 +96,8 @@ > import org.apache.catalina.core.StandardWrapper; > import org.apache.catalina.util.InstanceSupport; > import org.apache.catalina.util.StringManager; > - > +import org.apache.commons.logging.Log; > +import org.apache.commons.logging.LogFactory; > > /** > * Standard implementation of RequestDispatcher that allows a > @@ -181,13 +181,14 @@ > this.pathInfo = pathInfo; > this.queryString = queryString; > this.name = name; > + this.log = LogFactory.getLog( "ApplicationDispatcher[" + context.getPath() + "]" ); > if (wrapper instanceof StandardWrapper) > this.support = ((StandardWrapper) wrapper).getInstanceSupport(); > else > this.support = new InstanceSupport(wrapper); > > - if (debug >= 1) > - log("servletPath=" + this.servletPath + ", pathInfo=" + > + if (debug >= 1 && log.isDebugEnabled() ) > + log.debug("servletPath=" + this.servletPath + ", pathInfo=" + > this.pathInfo + ", queryString=" + queryString + > ", name=" + this.name); > > @@ -195,11 +196,12 @@ > // the request parameters appropriately > String jspFile = wrapper.getJspFile(); > if (jspFile != null) { > - if (debug >= 1) > - log("-->servletPath=" + jspFile); > + if (debug >= 1 && log.isDebugEnabled() ) > + log.debug("-->servletPath=" + jspFile); > this.servletPath = jspFile; > } > > + > } > > > @@ -312,6 +314,12 @@ > private ServletResponse wrapResponse = null; > > > + /** > + * where we log messages to > + */ > + private Log log = null; > + > + > // ------------------------------------------------------------- Properties > > > @@ -363,16 +371,16 @@ > > // Reset any output that has been buffered, but keep headers/cookies > if (response.isCommitted()) { > - if (debug >= 1) > - log(" Forward on committed response --> ISE"); > + if ( debug>=1 && log.isDebugEnabled() ) > + log.debug(" Forward on committed response --> ISE"); > throw new IllegalStateException > (sm.getString("applicationDispatcher.forward.ise")); > } > try { > response.resetBuffer(); > } catch (IllegalStateException e) { > - if (debug >= 1) > - log(" Forward resetBuffer() returned ISE: " + e); > + if (debug >= 1 && log.isDebugEnabled() ) > + log.debug(" Forward resetBuffer() returned ISE: " + e); > throw e; > } > > @@ -390,8 +398,8 @@ > // Handle a non-HTTP forward by passing the existing request/response > if ((hrequest == null) || (hresponse == null)) { > > - if (debug >= 1) > - log(" Non-HTTP Forward"); > + if (debug >= 1 && log.isDebugEnabled() ) > + log.debug(" Non-HTTP Forward"); > invoke(request, response); > > } > @@ -399,8 +407,8 @@ > // Handle an HTTP named dispatcher forward > else if ((servletPath == null) && (pathInfo == null)) { > > - if (debug >= 1) > - log(" Named Dispatcher Forward"); > + if (debug >= 1 && log.isDebugEnabled() ) > + log.debug(" Named Dispatcher Forward"); > invoke(request, response); > > } > @@ -408,8 +416,8 @@ > // Handle an HTTP path-based forward > else { > > - if (debug >= 1) > - log(" Path Based Forward"); > + if (debug >= 1 && log.isDebugEnabled() ) > + log.debug(" Path Based Forward"); > > ApplicationHttpRequest wrequest = > (ApplicationHttpRequest) wrapRequest(); > @@ -435,8 +443,8 @@ > } > > // Commit and close the response before we return > - if (debug >= 1) > - log(" Committing and closing response"); > + if (debug >= 1 && log.isDebugEnabled() ) > + log.debug(" Committing and closing response"); > > if (response instanceof ResponseFacade) { > ((ResponseFacade) response).finish(); > @@ -514,8 +522,8 @@ > if (!(request instanceof HttpServletRequest) || > !(response instanceof HttpServletResponse)) { > > - if (debug >= 1) > - log(" Non-HTTP Include"); > + if (debug >= 1 && log.isDebugEnabled() ) > + log.debug(" Non-HTTP Include"); > invoke(request, outerResponse); > unwrapResponse(); > > @@ -524,8 +532,8 @@ > // Handle an HTTP named dispatcher include > else if (name != null) { > > - if (debug >= 1) > - log(" Named Dispatcher Include"); > + if (debug >= 1 && log.isDebugEnabled() ) > + log.debug(" Named Dispatcher Include"); > > ApplicationHttpRequest wrequest = > (ApplicationHttpRequest) wrapRequest(); > @@ -541,8 +549,8 @@ > // Handle an HTTP path based include > else { > > - if (debug >= 1) > - log(" Path Based Include"); > + if (debug >= 1 && log.isDebugEnabled() ) > + log.debug(" Path Based Include"); > > ApplicationHttpRequest wrequest = > (ApplicationHttpRequest) wrapRequest(); > @@ -629,7 +637,8 @@ > > // Check for the servlet being marked unavailable > if (wrapper.isUnavailable()) { > - log(sm.getString("applicationDispatcher.isUnavailable", > + if ( log.isDebugEnabled() ) > + log.debug(sm.getString("applicationDispatcher.isUnavailable", > wrapper.getName())); > if (hresponse == null) { > ; // NOTE - Not much we can do generically > @@ -655,12 +664,14 @@ > // log(" No servlet instance returned!"); > } > } catch (ServletException e) { > - log(sm.getString("applicationDispatcher.allocateException", > + if (log.isDebugEnabled() ) > + log.debug(sm.getString("applicationDispatcher.allocateException", > wrapper.getName()), e); > servletException = e; > servlet = null; > } catch (Throwable e) { > - log(sm.getString("applicationDispatcher.allocateException", > + if (log.isDebugEnabled() ) > + log.debug(sm.getString("applicationDispatcher.allocateException", > wrapper.getName()), e); > servletException = new ServletException > (sm.getString("applicationDispatcher.allocateException", > @@ -694,14 +705,16 @@ > request.removeAttribute(Globals.JSP_FILE_ATTR); > support.fireInstanceEvent(InstanceEvent.AFTER_DISPATCH_EVENT, > servlet, request, response); > - log(sm.getString("applicationDispatcher.serviceException", > + if (log.isDebugEnabled() ) > + log.debug(sm.getString("applicationDispatcher.serviceException", > wrapper.getName()), e); > ioException = e; > } catch (UnavailableException e) { > request.removeAttribute(Globals.JSP_FILE_ATTR); > support.fireInstanceEvent(InstanceEvent.AFTER_DISPATCH_EVENT, > servlet, request, response); > - log(sm.getString("applicationDispatcher.serviceException", > + if (log.isDebugEnabled() ) > + log.debug(sm.getString("applicationDispatcher.serviceException", > wrapper.getName()), e); > servletException = e; > wrapper.unavailable(e); > @@ -709,14 +722,16 @@ > request.removeAttribute(Globals.JSP_FILE_ATTR); > support.fireInstanceEvent(InstanceEvent.AFTER_DISPATCH_EVENT, > servlet, request, response); > - log(sm.getString("applicationDispatcher.serviceException", > + if (log.isDebugEnabled() ) > + log.debug(sm.getString("applicationDispatcher.serviceException", > wrapper.getName()), e); > servletException = e; > } catch (RuntimeException e) { > request.removeAttribute(Globals.JSP_FILE_ATTR); > support.fireInstanceEvent(InstanceEvent.AFTER_DISPATCH_EVENT, > servlet, request, response); > - log(sm.getString("applicationDispatcher.serviceException", > + if (log.isDebugEnabled() ) > + log.debug(sm.getString("applicationDispatcher.serviceException", > wrapper.getName()), e); > runtimeException = e; > } > @@ -729,11 +744,13 @@ > wrapper.deallocate(servlet); > } > } catch (ServletException e) { > - log(sm.getString("applicationDispatcher.deallocateException", > + if (log.isDebugEnabled() ) > + log.debug(sm.getString("applicationDispatcher.deallocateException", > wrapper.getName()), e); > servletException = e; > } catch (Throwable e) { > - log(sm.getString("applicationDispatcher.deallocateException", > + if (log.isDebugEnabled() ) > + log.debug(sm.getString("applicationDispatcher.deallocateException", > wrapper.getName()), e); > servletException = new ServletException > (sm.getString("applicationDispatcher.deallocateException", > @@ -751,45 +768,6 @@ > throw servletException; > if (runtimeException != null) > throw runtimeException; > - > - } > - > - > - /** > - * Log a message on the Logger associated with our Context (if any) > - * > - * @param message Message to be logged > - */ > - private void log(String message) { > - > - Logger logger = context.getLogger(); > - if (logger != null) > - logger.log("ApplicationDispatcher[" + context.getPath() + > - "]: " + message); > - else > - System.out.println("ApplicationDispatcher[" + > - context.getPath() + "]: " + message); > - > - } > - > - > - /** > - * Log a message on the Logger associated with our Container (if any) > - * > - * @param message Message to be logged > - * @param throwable Associated exception > - */ > - private void log(String message, Throwable throwable) { > - > - Logger logger = context.getLogger(); > - if (logger != null) > - logger.log("ApplicationDispatcher[" + context.getPath() + > - "] " + message, throwable); > - else { > - System.out.println("ApplicationDispatcher[" + > - context.getPath() + "]: " + message); > - throwable.printStackTrace(System.out); > - } > > } > > > > -- > To unsubscribe, e-mail: > For additional commands, e-mail: > > -- To unsubscribe, e-mail: For additional commands, e-mail: