Lloyd,
I am reviewing the SunoneInterceptor changes soon, but here is
this review first ...
See inline.
Another request -- send cvs diff "-u", please.
> AMXLoggingHook.getInstance().enableLoggingHook() is now called from
> AdminService.start(). This avoids recursive logging problems with
> trying to fully initialize AMXLogging before PEMain.main() has run.
> The change also eliminates the somewhat convoluted initialization
> sequence for AMXLogging.
>
Recursive logging problems? That sounds serious. You mean to
say we have those at the moment?
>
> Lloyd
>
>
> RCS file:
> /cvs/glassfish/appserv-core/src/java/com/sun/enterprise/server/logging/FileandSyslogHandler.java,v
>
> retrieving revision 1.6
> diff -r1.6 FileandSyslogHandler.java
> 251c251
> < mAMXLoggingHook = createAMXLoggingHook();
> ---
> > mAMXLoggingHook = AMXLoggingHook.getInstance();
>
OK.
>
> MB2:/gf/build/glassfish lloyd$ cvs diff -u -w
> appserv-core/src/java/com/sun/enterprise/server/logging/AMXLoggingHook.java
>
> Index:
> appserv-core/src/java/com/sun/enterprise/server/logging/AMXLoggingHook.java
>
> ===================================================================
> RCS file:
> /cvs/glassfish/appserv-core/src/java/com/sun/enterprise/server/logging/AMXLoggingHook.java,v
>
> retrieving revision 1.4
> diff -u -w -r1.4 AMXLoggingHook.java
> ---
> appserv-core/src/java/com/sun/enterprise/server/logging/AMXLoggingHook.java
> 25 Dec 2005 04:16:26 -0000 1.4
> +++
> appserv-core/src/java/com/sun/enterprise/server/logging/AMXLoggingHook.java
> 6 Jan 2007 14:49:36 -0000
> @@ -34,16 +34,19 @@
> import java.lang.reflect.Method;
> import java.lang.reflect.Constructor;
> +import com.sun.enterprise.util.FeatureAvailability;
> +
> +import com.sun.appserv.management.helper.AMXDebugHelper;
> +
> /**
> Hook used for AMX logging by FileandSysLogHandler.
> <b>This class assumes that the caller is holding a lock that
> will prevent more than one thread from calling {_at_link #publish}.</b>
> */
> -final class AMXLoggingHook // don't make this class public
> +public final class AMXLoggingHook // don't make this class public
Remove the comment. This class IS public.
> {
> private ObjectName mLoggingObjectName;
> private MBeanServer mMBeanServer;
> - private boolean mLoggingAvailable;
> private LoggingImplHook mLoggingImplHook;
>
> /**
> @@ -54,19 +57,11 @@
> */
> private Level mMinimumLogLevel;
>
> - // /** signature for LOGGING_HOOK_METHOD_NAME method */
> - // private static final String[] LOGGING_HOOK_SIG = new String[]
> - // { LogRecord.class.getName(), Formatter.class.getName() };
> -
> - // /** name of the operation called on the AMX Logging MBean */
> - // private static final String LOGGING_HOOK_NAME =
> "privateLoggingHook";
> -
> - private static final boolean DEBUG = false;
> - private final Output mOut;
> + private final AMXDebugHelper mDebug;
> private final String mServerName;
>
> private void
> - dumpSystemProps( final Output output )
> + dumpSystemProps( final AMXDebugHelper output )
> {
> final java.util.Properties props = System.getProperties();
>
> @@ -79,93 +74,90 @@
>
> }
>
> + private static final AMXLoggingHook INSTANCE = new AMXLoggingHook();
> +
> + public static AMXLoggingHook
> + getInstance()
> + {
> + return INSTANCE;
> + }
> +
> AMXLoggingHook()
> {
> - mServerName = System.getProperty(
> -
> com.sun.enterprise.util.SystemPropertyConstants.SERVER_NAME);
> + mServerName = System.getProperty(
> com.sun.enterprise.util.SystemPropertyConstants.SERVER_NAME);
> final String instanceRoot = System.getProperty(
>
> com.sun.enterprise.util.SystemPropertyConstants.INSTANCE_ROOT_PROPERTY );
>
> - mOut = createOutput( DEBUG, instanceRoot +
> "/AMXLoggingHook-" + mServerName + ".debug" );
> + mDebug = new AMXDebugHelper( instanceRoot +
> "/AMXLoggingHook-" + mServerName + ".debug" );
> +
> // debug(
> "\n_______________________________________________________________________________");
>
> // debug( "AMXLoggingHook: REV = 1");
> // debug( "AMXLoggingHook: started at " + new
> java.util.Date() + " for server " + mServerName);
>
> - // dumpSystemProps( mOut );
> + // dumpSystemProps( mDebug );
> mLoggingObjectName = null;
> - mMBeanServer = getMBeanServer(); // may or may not be
> available
> -
> mMinimumLogLevel = Level.FINEST;
> mLoggingImplHook = null;
>
> - }
> -
> - private static Output
> - createOutput( final boolean debug, final String fileName )
> - {
> - try
> - {
> - final java.io.File f = new java.io.File( fileName );
> + //mDebug.println(
> com.sun.appserv.management.util.misc.ExceptionUtil.toString( new
> Exception("HELLO") ) );
>
> - return debug ? new FileOutput( f ) : new NullOutput();
> - }
> - catch( Throwable t )
> - {
> - // nothing can be done; fall through and return NullOutput
> + // can't initialize now; FileandSysLogHandler is called even
> before main()
> + mMBeanServer = null;
> }
> + private final void debug( final Object o ) { mDebug.println( o
> ); }
A private method is always final, isn't it or am I misreading the JLS?
>
> - return new NullOutput();
> - }
> + private static final String LOGGING_IMPL_CLASSNAME =
> + "com.sun.enterprise.management.ext.logging.LoggingImpl";
>
> - private final void debug( final Object o ) { mOut.println(
> o.toString() ); }
> - private MBeanServer
> - getMBeanServer()
> - {
> - if ( mMBeanServer == null )
> - {
> - mMBeanServer =
> -
> com.sun.enterprise.admin.common.MBeanServerFactory.getMBeanServer();
> - // may still be null if not yet available
> - if ( mMBeanServer != null )
> + /**
> + Called exactly once to install the Logging MBean and turn on
> AMX support for logging.
> + @return ObjectName of AMX Logging MBean
> + */
> + public static ObjectName
> + enableLoggingHook()
> {
> - debug( "MBeanServer NOW EXISTS, creating LoggingImpl" );
> - initLogging( mMBeanServer );
> - }
> + return getInstance()._enableLoggingHook();
> }
>
> - return mMBeanServer;
> + private synchronized ObjectName
> + _enableLoggingHook()
> + {
> + debug( "_enableLoggingHook" );
> + if ( mLoggingImplHook != null )
> + {
> + throw new IllegalStateException();
> }
>
> + mMBeanServer = (MBeanServer)
> + FeatureAvailability.getInstance().waitForFeature(
> FeatureAvailability.MBEAN_SERVER_FEATURE, "AMXLoggingHook");
>
> - private static final String LOGGING_IMPL_CLASSNAME =
> - "com.sun.enterprise.management.ext.logging.LoggingImpl";
> - /**
> - Called once to install the Logging MBean
> - */
> - private void
> - initLogging( final MBeanServer mbeanServer )
> - {
> + LoggingImplHook hook = null;
> try
> {
> final Class loggingClass = Class.forName(
> LOGGING_IMPL_CLASSNAME );
>
> final Constructor constructor =
> loggingClass.getConstructor( String.class );
> - mLoggingImplHook =
> (LoggingImplHook)constructor.newInstance( mServerName );
> + hook = (LoggingImplHook)constructor.newInstance(
> mServerName );
>
> final Method getObjectNameMethod =
> loggingClass.getMethod( "getObjectName", String.class );
> - mLoggingObjectName =
> (ObjectName)getObjectNameMethod.invoke( mLoggingImplHook, mServerName );
> - debug( "registering Logging as: " + mLoggingObjectName );
> - mbeanServer.registerMBean( mLoggingImplHook,
> mLoggingObjectName );
> + final ObjectName proposedObjectName =
> (ObjectName)getObjectNameMethod.invoke( hook, mServerName );
> + debug( "registering Logging as: " + proposedObjectName );
> + mLoggingObjectName = mMBeanServer.registerMBean( hook,
> proposedObjectName ).getObjectName();
> + mLoggingImplHook = hook;
> }
> catch ( Exception e )
> {
> + hook = null;
> final String msg = "Can't load " + LOGGING_IMPL_CLASSNAME
> + ", caught: " + e;
> debug( msg );
> - throw new Error( msg );
> + throw new Error( msg, e);
You are throwing an "Error"?
Why?
>
Please correct the above.
Rest looks all right.
Thanks,
Kedar