commons-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Simon Kitching <skitch...@apache.org>
Subject Re: [VOTE] Release RC10 As Commons Logging 1.1
Date Tue, 02 May 2006 10:27:05 GMT
Hi,

To save people reading through below, my summary is:

<summary>
Some excellent proofreading here, and a number of doc/release-notes
issues have been found by Dennis, as well as a case where we could use
constants instead of inline strings, and one where we should technically
be using a param value instead of a constant in a diagnostic message
(though it doesn't matter in that case anyway).

However I don't see anything here that I think is worth cancelling the
RC10 vote for. A few items would be good to put up on the wiki under
"1.1 release addendum".
</summary>

On Tue, 2006-05-02 at 00:20 +0200, Dennis Lundberg wrote:
> robert burrell donkin wrote:
> > after some investigation, the issue which turned up with RC8 was
> > determined not to be a bug. notes have been added to the troubleshooting
> > documentation. 
> > 
> > RC10 is available from:
> > http://people.apache.org/~rdonkin/commons-logging. please download,
> > check and then vote.
> 
> <snip>
> 
> Sorry to be jumping into this at the last minute. I've done a code 
> review of /src/java and /xdocs and compared them to the 1.0.4 release. 
> As you might have seen I committed some spelling corrections to the 
> xdocs. The added documents are excellent by the way!

Thanks for the doc patches. It's amazing how mistakes creep in!

> 
> There were a couple of questions that popped up along the way, which I 
> will deal with below, one file at a time. Nothing big, mostly 
> documentation stuff, so nothing to hold up a release.
> 
> The attached patch file fixes a couple of them, while others need input 
> from you. I didn't want to commit the patch before I had checked it with 
> the rest of you.
> 
> I hope to be doing some practical tests in applet environments tomorrow.
> 
> 
> \commons-logging-1.1-RC10-src\RELEASE-NOTES.txt
> 
> Lines 147-150:
> "Previous releases of commons-logging-api.jar contained the Jdk14Logger 
> class;
> this is now deprecated. It will be removed from the API jar in some future
> release. If your application needs this jar, then instead of
> upgrading to commons-logging-api-1.1.jar, upgrade to 
> commons-logging-1.1.jar."
> 
> I don't see why we advice this. The jar still contains Jdk14Logger. 
> Should we encourage people to switch jars now or when JCL 2 comes out?

We did want to remove Jdk14Logger from the api jar because it just
doesn't belong in an "api" jar. 

However it later became evident that using a full JCL jar doesn't work
with Tomcat because tomcat loads JCL via the system classloader, and
also prevents any class loaded via that classloader from being
overridden by a webapp. We could provide another jar for this sort of
situation (commons-logging-tomcat.jar or similar name), but it's
probably more sensible to just live with a weird "api" jar that contains
Jdk14Logger until the 2.x series (as you say).

Unfortunately it looks like the release-notes weren't updated (probably
my fault). So that advice is actually wrong when applied to tomcat, but
good in other cases. 

I think we could just make a note in the wiki about this misleading info
in the release notes. There is already some info on this tomcat issue
elsewhere in the JCL docs.


> Lines 173-174:
> "File commons-logging-api-nn.jar provides no adapters to external logging
> libraries, just the internally implemented SimpleLog and NoOpLog classes."
> 
> This is not entirely true. The Jdk14Logger is there as well.

Yep, due to the reversal during the RC series where we agreed to
reinstate that class. The release notes are wrong, but it's not too
serious. Actually, if you take literally the statement "adapters to
external logging libraries", java.util.logging may not qualify :-).

I suggest a wiki note could again cover this.

> There is nothing in the release notes about the fact that AvalonLogger 
> no longer implements Serializable. Do we need that?

Yes, the release notes probably should have pointed that out. The code
change is right; AvalonLogger never did serialize properly. In fact,
it's actually impossible given the design of AvalonLogger. As it never
worked, we can't be breaking any users by removing the feature.

This missing info in the release notes is not worth holding a release
back I think, esp. as it's obvious no-one ever tried to serialize an
AvalonLogger object.

> 
> 
> \commons-logging-1.1-RC10-src\src\java\org\apache\commons\logging\impl\AvalonLogger.java
> 
> Line 251: Javadoc for trace(Object message, Throwable t) was changed 
> like this:
>    @see org.apache.commons.logging.Log#trace(java.lang.Object, 
> java.lang.Throwable)  --->  @see 
> org.apache.commons.logging.Log#debug(Object, Throwable)
>    Should be changed back.
>    (done in patch)

Agreed. But not worth holding back a release for IMO.

> 
> 
> \commons-logging-1.1-RC10-src\src\java\org\apache\commons\logging\impl\Jdk13LumberjackLogger.java
> 
> Line 42: A since-tag of "1.1" was added, but the class existed in SCM 
> when the previous version was released, but was not included in the jar. 
> Should the since tag be there?

I think that the since tag should indicate what *release* it was in, ie
that "1.1" is the right thing.

> 
> 
> \commons-logging-1.1-RC10-src\src\java\org\apache\commons\logging\impl\LogFactoryImpl.java
> 
> Line 121: The sentence seems to not have been finished...

Yep. Should be something like:
  When set to false, a LogConfigurationException is thrown if
  LogFactoryImpl is loaded via a child classloader of the TCCL (this  
  should never happen in sane systems).

> Lines 173-175: Could use the constants that were defined in lines 74-78.

Yep, agreed (the constants were added later, to support diagnostic
output).

> Line 761,1322: Missing indentation
>    (done in patch)
Actually, that was deliberate. I prefer that layout, where the throws
clause is not indented:
  public void foo(.......)
  throws BarException {
    // first line
  }
However I can live with the (more common) proposed format:
  public void foo(.......)
       throws BarException {
    // first line
  }

> Line 1252: Should be removed, copy-and-paste error
>    (done in patch)
> 

Agreed (just a comment).



> 
> \commons-logging-1.1-RC10-src\src\java\org\apache\commons\logging\impl\SimpleLog.java
> 
> Line 398: The javadoc below
>    @see org.apache.commons.logging.Log#trace(Object, Throwable)
> should be
>    @see org.apache.commons.logging.Log#trace(Object)
>    (done in patch)
> 
> \commons-logging-1.1-RC10-src\src\java\org\apache\commons\logging\LogFactory.java
> Line 142: Should be changed
>    </code></pre>  --->  </pre></code>
>    (done in patch)
> Lines 335-340, 1130, 1219-1271: Indentation is wrong (is using 
> tab-characters)
>    (done in patch)
> Line 574: Should this be converted into a @todo?

Yes, possibly:
  // TODO: think whether we need to handle exceptions from newFactory..

> Line 1172: LogFactory.class.getClassLoader.load(name)  ---> 
> LogFactory.class.getClassLoader().load(name)
>    (done in patch)
> Line 1217: The last part of the JavaDocs are cut in the middle of a 
> sentence. What should it say?

 * @return true if the <code>logFactoryClass</code> does extend
 * <code>LogFactory</code> when that class is loaded
 * via the same classloader that loaded the
 * <code>logFactoryClass</code>.

> Lines 1462, 1466: The file doesn't have to be called FACTORY_PROPERTIES, 
> the name is given by the parameter "fileName". The method is private and 
> is only called once with fileName=FACTORY_PROPERTIES. Should this be 
> changed?

Yes, 1462/1466 should use the fileName parameter when outputting their
diagnostic messages. Not serious, though because as you point out, the
fileName parameter always *is* FACTORY_PROPERTIES.

> Line 1477: OUTPUT_PROPERTY  --->  DIAGNOSTICS_DEST_PROPERTY or perhaps 
> "org.apache.commons.logging.diagnostics.dest". Which one is best?

Yep, javadoc wasn't updated. I think DIAGNOSTICS_DEST_PROPERTY is best;
it is a public constant so the user can see what real value is needed.

> Line 1655: Missing a space at the end of the string (after the colon)
>    (done in patch)
> 
> 
> \commons-logging-1.1-RC10-src\xdocs\guide.xml
> Line 150: Remove " in each file".
>    (done in patch)
> 
> 
> \commons-logging-1.1-RC10-src\xdocs\tech.xml
> Line 649:
>    "JCL uses the context classloader to use the <code>Log</code> 
> implementation."
> What should it be?
>    "JCL uses the context classloader to get|load the <code>Log</code> 
> implementation."

I think:
  "to load the <code>Log</code> implementation".
We really do call ClassLoader.loadClass, so "load" seems right.

> 
> 
> \commons-logging-1.1-RC10-src\xdocs\troubleshooting.xml
> Line 91:
>    "Each diagnostic message is prefixed with details of the class being 
> logger in a standard format."
> What should it be?
>    "Each diagnostic message is prefixed with details of the class being 
> logged|loaded|diagnosed in a standard format."
> 

Not sure. How about this?
  "prefixed with details of the relevant class in a standard format."



Dennis, I'm really impressed by such careful inspection of the code!
Thanks for all the comments.

I'm +1 to the attached patch being applied. If you want to take care of
the other things mentioned here, great. Otherwise I will deal with them.


However I don't see anything here that's worth cancelling the RC10 vote
for. A few items would be good to put up on the wiki as "1.1 release
addendum".

Cheers,

Simon


========= (original patch follows)

> plain text document attachment (jcl-1.1-rc10.patch)
> Index: xdocs/guide.xml
> ===================================================================
> --- xdocs/guide.xml	(revision 398696)
> +++ xdocs/guide.xml	(arbetskopia)
> @@ -147,7 +147,7 @@
>  When such a file exists, every entry in the properties file becomes an "attribute"
>  of the LogFactory. When there is more than one such file in the classpath, releases
>  of commons-logging prior to 1.1 simply use the first one found. From release 1.1,
> -each file may define a <code>priority</code> key in each file, and the file
with
> +each file may define a <code>priority</code> key, and the file with
>  the highest priority is used (no priority definition implies priority of zero).
>  When multiple files have the same priority, the first one found is used.
>  </p>
> Index: src/java/org/apache/commons/logging/impl/AvalonLogger.java
> ===================================================================
> --- src/java/org/apache/commons/logging/impl/AvalonLogger.java	(revision 398661)
> +++ src/java/org/apache/commons/logging/impl/AvalonLogger.java	(arbetskopia)
> @@ -248,7 +248,7 @@
>       * 
>       * @param message to log.
>       * @param t log this cause.
> -     * @see org.apache.commons.logging.Log#debug(Object, Throwable)
> +     * @see org.apache.commons.logging.Log#trace(Object, Throwable)
>       */
>      public void trace(Object message, Throwable t) {
>          if (getLogger().isDebugEnabled()) getLogger().debug(String.valueOf(message),
t);
> Index: src/java/org/apache/commons/logging/impl/LogFactoryImpl.java
> ===================================================================
> --- src/java/org/apache/commons/logging/impl/LogFactoryImpl.java	(revision 398661)
> +++ src/java/org/apache/commons/logging/impl/LogFactoryImpl.java	(arbetskopia)
> @@ -758,8 +758,7 @@
>       * or if no adapter at all can be instantiated
>       */
>      private Log discoverLogImplementation(String logCategory)
> -    throws LogConfigurationException
> -    {
> +            throws LogConfigurationException {
>          if (isDiagnosticsEnabled()) {
>              logDiagnostic("Discovering a Log implementation...");
>          }
> @@ -1248,8 +1247,7 @@
>              current = current.getParent();
>          }
>         
> -       // scan c2's ancestors to find c1
> -        // scan c1's ancestors to find c2
> +        // scan c2's ancestors to find c1
>          current = c2;
>          while (current != null) {
>              if (current == c1)
> @@ -1319,7 +1317,7 @@
>       * should not be recovered from.
>       */
>      private void handleFlawedHierarchy(ClassLoader badClassLoader, Class badClass)
> -    throws LogConfigurationException {
> +            throws LogConfigurationException {
>  
>          boolean implementsLog = false;
>          String logInterfaceName = Log.class.getName();
> Index: src/java/org/apache/commons/logging/impl/SimpleLog.java
> ===================================================================
> --- src/java/org/apache/commons/logging/impl/SimpleLog.java	(revision 398661)
> +++ src/java/org/apache/commons/logging/impl/SimpleLog.java	(arbetskopia)
> @@ -395,7 +395,7 @@
>       * <code>org.apache.commons.logging.impl.SimpleLog.LOG_LEVEL_TRACE</code>.
>       *
>       * @param message to log
> -     * @see org.apache.commons.logging.Log#trace(Object, Throwable)
> +     * @see org.apache.commons.logging.Log#trace(Object)
>       */
>      public final void trace(Object message) {
>  
> Index: src/java/org/apache/commons/logging/LogFactory.java
> ===================================================================
> --- src/java/org/apache/commons/logging/LogFactory.java	(revision 398661)
> +++ src/java/org/apache/commons/logging/LogFactory.java	(arbetskopia)
> @@ -139,7 +139,7 @@
>       * <strong>Note:</strong> <code>LogFactory</code> will print:
>       * <code><pre>
>       * [ERROR] LogFactory: Load of custom hashtable failed</em>
> -     * </code></pre>
> +     * </pre></code>
>       * to system error and then continue using a standard Hashtable.
>       * </p>
>       * <p>
> @@ -328,16 +328,16 @@
>          } catch (Throwable t) {
>              // ignore
>              if (!WEAK_HASHTABLE_CLASSNAME.equals(storeImplementationClass)) {
> -        	    // if the user's trying to set up a custom implementation, give a clue
> -        	    if (isDiagnosticsEnabled()) {
> -        	        // use internal logging to issue the warning
> -        	        logDiagnostic("[ERROR] LogFactory: Load of custom hashtable failed");
> -        		} else {
> -        		    // we *really* want this output, even if diagnostics weren't
> +                // if the user's trying to set up a custom implementation, give a clue
> +                if (isDiagnosticsEnabled()) {
> +                    // use internal logging to issue the warning
> +                    logDiagnostic("[ERROR] LogFactory: Load of custom hashtable failed");
> +                } else {
> +                    // we *really* want this output, even if diagnostics weren't
>                      // explicitly enabled by the user.
> -        		    System.err.println("[ERROR] LogFactory: Load of custom hashtable failed");
> -        		}
> -        	}
> +                    System.err.println("[ERROR] LogFactory: Load of custom hashtable
failed");
> +                }
> +            }
>          }
>          if (result == null) {
>              result = new Hashtable();
> @@ -1127,7 +1127,7 @@
>                          // to their native logging system. 
>                          // 
>                          String msg = 
> -                        	"The application has specified that a custom LogFactory implementation
should be used but " +
> +                            "The application has specified that a custom LogFactory
implementation should be used but " +
>                              "Class '" + factoryClass + "' cannot be converted to '"
>                              + LogFactory.class.getName() + "'. ";
>                          if (implementsLogFactory) {
> @@ -1169,7 +1169,7 @@
>               * classLoader was unable to load factoryClass.
>               *
>               * In either case, we call Class.forName, which is equivalent
> -             * to LogFactory.class.getClassLoader.load(name), ie we ignore
> +             * to LogFactory.class.getClassLoader().load(name), ie we ignore
>               * the classloader parameter the caller passed, and fall back
>               * to trying the classloader associated with this class. See the
>               * javadoc for the newFactory method for more info on the 
> @@ -1216,59 +1216,59 @@
>       * @param logFactoryClass <code>Class</code> which may implement <code>LogFactory</code>
>       * @return true if the <code>Class</code> is assignable from the 
>       */
> -	private static boolean implementsLogFactory(Class logFactoryClass) {
> -		boolean implementsLogFactory = false;
> -		if (logFactoryClass != null) {
> -			try {
> -			    ClassLoader logFactoryClassLoader = logFactoryClass.getClassLoader();
> -			    if (logFactoryClassLoader == null) {
> -			        logDiagnostic("[CUSTOM LOG FACTORY] was loaded by the boot classloader");
> -			    } else {
> -			        logHierarchy("[CUSTOM LOG FACTORY] ", logFactoryClassLoader);
> -			        Class factoryFromCustomLoader 
> -			        	= Class.forName("org.apache.commons.logging.LogFactory", false, logFactoryClassLoader);
> -			        implementsLogFactory = factoryFromCustomLoader.isAssignableFrom(logFactoryClass);
> -			        if (implementsLogFactory) {
> -			        	logDiagnostic("[CUSTOM LOG FACTORY] " + logFactoryClass.getName() 
> -			        			+ " implements LogFactory but was loaded by an incompatible classloader.");
> -			        } else {
> -			        	logDiagnostic("[CUSTOM LOG FACTORY] " + logFactoryClass.getName() 
> -			        			+ " does not implement LogFactory.");
> -			        }
> -			    }
> -			} catch (SecurityException e) {
> -				//
> -				// The application is running within a hostile security environment.
> -				// This will make it very hard to diagnose issues with JCL.
> -				// Consider running less securely whilst debugging this issue.
> -				//
> -				logDiagnostic("[CUSTOM LOG FACTORY] SecurityException thrown whilst trying to determine
whether " +
> -						"the compatibility was caused by a classloader conflict: " 
> -						+ e.getMessage());
> -			} catch (LinkageError e) {
> -				//
> -				// This should be an unusual circumstance.
> -				// LinkageError's usually indicate that a dependent class has incompatibly changed.
> -				// Another possibility may be an exception thrown by an initializer.
> -				// Time for a clean rebuild?
> -				//
> -				logDiagnostic("[CUSTOM LOG FACTORY] LinkageError thrown whilst trying to determine
whether " +
> -						"the compatibility was caused by a classloader conflict: " 
> -						+ e.getMessage());
> -			} catch (ClassNotFoundException e) {
> -				//
> -				// LogFactory cannot be loaded by the classloader which loaded the custom factory
implementation.
> -				// The custom implementation is not viable until this is corrected. 
> -				// Ensure that the JCL jar and the custom class are available from the same classloader.
> -				// Running with diagnostics on should give information about the classloaders used

> -				// to load the custom factory.
> -				// 
> -				logDiagnostic("[CUSTOM LOG FACTORY] LogFactory class cannot be loaded by classloader
which loaded the " +
> -						"custom LogFactory implementation. Is the custom factory in the right classloader?");
> -			}
> -		}
> -		return implementsLogFactory;
> -	}
> +    private static boolean implementsLogFactory(Class logFactoryClass) {
> +        boolean implementsLogFactory = false;
> +        if (logFactoryClass != null) {
> +            try {
> +                ClassLoader logFactoryClassLoader = logFactoryClass.getClassLoader();
> +                if (logFactoryClassLoader == null) {
> +                    logDiagnostic("[CUSTOM LOG FACTORY] was loaded by the boot classloader");
> +                } else {
> +                    logHierarchy("[CUSTOM LOG FACTORY] ", logFactoryClassLoader);
> +                    Class factoryFromCustomLoader
> +                        = Class.forName("org.apache.commons.logging.LogFactory", false,
logFactoryClassLoader);
> +                    implementsLogFactory = factoryFromCustomLoader.isAssignableFrom(logFactoryClass);
> +                    if (implementsLogFactory) {
> +                        logDiagnostic("[CUSTOM LOG FACTORY] " + logFactoryClass.getName()
> +                                        + " implements LogFactory but was loaded by
an incompatible classloader.");
> +                    } else {
> +                        logDiagnostic("[CUSTOM LOG FACTORY] " + logFactoryClass.getName()
> +                                        + " does not implement LogFactory.");
> +                    }
> +                }
> +            } catch (SecurityException e) {
> +                //
> +                // The application is running within a hostile security environment.
> +                // This will make it very hard to diagnose issues with JCL.
> +                // Consider running less securely whilst debugging this issue.
> +                //
> +                logDiagnostic("[CUSTOM LOG FACTORY] SecurityException thrown whilst
trying to determine whether " +
> +                                "the compatibility was caused by a classloader conflict:
"
> +                                + e.getMessage());
> +            } catch (LinkageError e) {
> +                //
> +                // This should be an unusual circumstance.
> +                // LinkageError's usually indicate that a dependent class has incompatibly
changed.
> +                // Another possibility may be an exception thrown by an initializer.
> +                // Time for a clean rebuild?
> +                //
> +                logDiagnostic("[CUSTOM LOG FACTORY] LinkageError thrown whilst trying
to determine whether " +
> +                                "the compatibility was caused by a classloader conflict:
"
> +                                + e.getMessage());
> +            } catch (ClassNotFoundException e) {
> +                //
> +                // LogFactory cannot be loaded by the classloader which loaded the custom
factory implementation.
> +                // The custom implementation is not viable until this is corrected.
> +                // Ensure that the JCL jar and the custom class are available from the
same classloader.
> +                // Running with diagnostics on should give information about the classloaders
used
> +                // to load the custom factory.
> +                //
> +                logDiagnostic("[CUSTOM LOG FACTORY] LogFactory class cannot be loaded
by classloader which loaded the " +
> +                                "custom LogFactory implementation. Is the custom factory
in the right classloader?");
> +            }
> +        }
> +        return implementsLogFactory;
> +    }
>  
>      /**
>       * Applets may run in an environment where accessing resources of a loader is
> @@ -1652,7 +1652,7 @@
>              return;
>          }        
>          if (classLoader != null) {
> -            StringBuffer buf = new StringBuffer(prefix + "ClassLoader tree:");
> +            StringBuffer buf = new StringBuffer(prefix + "ClassLoader tree: ");
>              for(;;) {
>                  buf.append(objectId(classLoader));
>                  if (classLoader == systemClassLoader) {
> 
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: commons-dev-unsubscribe@jakarta.apache.org
> For additional commands, e-mail: commons-dev-help@jakarta.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: commons-dev-unsubscribe@jakarta.apache.org
For additional commands, e-mail: commons-dev-help@jakarta.apache.org


Mime
View raw message