tomcat-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Craig R. McClanahan" <craig...@apache.org>
Subject Re: [5] commons-logging
Date Tue, 06 Aug 2002 20:28:55 GMT


On 6 Aug 2002, Bob Herrmann wrote:

> Date: 06 Aug 2002 16:19:54 -0400
> From: Bob Herrmann <bob@jadn.com>
> Reply-To: Tomcat Developers List <tomcat-dev@jakarta.apache.org>
> To: Tomcat Developers List <tomcat-dev@jakarta.apache.org>
> 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 <code>RequestDispatcher</code> 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:   <mailto:tomcat-dev-unsubscribe@jakarta.apache.org>
> For additional commands, e-mail: <mailto:tomcat-dev-help@jakarta.apache.org>
>
>


--
To unsubscribe, e-mail:   <mailto:tomcat-dev-unsubscribe@jakarta.apache.org>
For additional commands, e-mail: <mailto:tomcat-dev-help@jakarta.apache.org>


Mime
View raw message