Hi, Marina
Comments inline..
On 9/13/06, Marina Vatkina <Marina.Vatkina_at_sun.com> wrote:
>
> Tom, Wonseok,
>
> So that the patch doesn't get lost in myriad of other changes of a single
> checkin, I can do it separately.
>
> I have couple of code-related questions:
>
> 1. The new method name: 'getSessionLog' - should it be
> 'getDefaultSessionLog'
> as it's not really a session logger?
There is a class "DefaultSessionLog", so getDefaultSessionLog() is also
confusing I think. How about getSessionLogFromProperty()? or please suggest
better one.
2. The original code:
> session.setSessionLog(serverPlatform.getServerLog());
> sets a new instance of the log into the session.
> The new code:
> session.setSessionLog(sessionLog);
> sets the existing logger into the session. This needs to be fixed, right?
You're right.
I reused the sessionLog instance (got from serverPlaform or getSessionLog())
for AbstractSessionLog.setLog() and session.setSessionLog() as I thought it
will have same state.
But AbstractSessionLog's default logger has null session but session's
logger has a session... different state. I'll fix it.
Wonseok,
> Did you run entity-persistence-tests and QLook with your changes?
I will run tests after fixing above...
thanks,
> -marina
>
> Tom Ware wrote:
> > Hi Wonseok,
> >
> > The proposal and implementation look good. How would you like us to
> > proceed? Here are the options I see:
> >
> > 1. I can include these changes as part of the next entity-persistence
> > update we do from Oracle
> > 2. One of the folks at Sun can check this in when they have time
> >
> > -tom
> >
> > Wonseok Kim wrote:
> >
> >> According to my proposal, I implemented this feature.
> >> Please review the code below(or see the attached file). Any comments
> >> about the code and the proposal are welcome.
> >>
> >> Summary:
> >> * The implementation for this property is very similar to the impl for
> >> "toplink.target-server" property.
> >> * Added oracle.toplink.essentials.config.LoggerType class for logical
> >> values.
> >> * Added a new property in TopLinkProperties and a nested
> >> LoggerTypeProp class in PropertiesHandler.
> >> * The logger is set in EntityManagerSetupImpl.predeploy() like
> >> ServerPlatform.getServerLog().
> >> * Added an exception message for instantiation failure of the logger
> >> class.
> >>
> >> Diff:
> >> ? src/java/oracle/toplink/essentials/config/LoggerType.java
> >>
> >> package oracle.toplink.essentials.config;
> >>
> >> /**
> >> * Logger type persistence property values.
> >> * <p/>
> >> * JPA persistence property Usage:
> >> * <blockquote>
> >> * properties.add(TopLinkProperties.LoggerType, LoggerType.JavaLogger);
> >> * </blockquote>
> >> * Property values are case-insensitive.
> >> *
> >> * @author Wonseok Kim
> >> */
> >> public class LoggerType {
> >> public static final String DefaultLogger = "DefaultLogger";
> >> public static final String JavaLogger = "JavaLogger";
> >>
> >> public static final String DEFAULT = DefaultLogger;
> >>
> >> }
> >>
> >> Index: src/java/oracle/toplink/essentials/config/TopLinkProperties.java
> >> ===================================================================
> >> RCS file:
> >>
> /cvs/glassfish/entity-persistence/src/java/oracle/toplink/essentials/config/TopLinkProperties.java,v
> >>
> >> retrieving revision 1.1
> >> diff -c -r1.1 TopLinkProperties.java
> >> ***
> >> src/java/oracle/toplink/essentials/config/TopLinkProperties.java 11
> >> May 2006 19:30:27 -0000 1.1
> >> ---
> >> src/java/oracle/toplink/essentials/config/TopLinkProperties.java 12
> >> Sep 2006 05:48:49 -0000
> >> ***************
> >> *** 83,89 ****
> >> public static final String CACHE_SHARED_DEFAULT = CACHE_SHARED_
> >> + DEFAULT;
> >>
> >> // Customizations properties
> >> ! // Valid values are names of levels defined in
> >> java.util.logging.Level ,
> >> // default value is java.util.logging.Level.CONFIG.getName()
> >> public static final String LOGGING_LEVEL = "toplink.logging.level
> ";
> >> --- 83,93 ----
> >> public static final String CACHE_SHARED_DEFAULT = CACHE_SHARED_
> >> + DEFAULT;
> >>
> >> // Customizations properties
> >> !
> >> ! // The type of logger. By default DefaultSessionLog is used.
> >> ! // Valid values are the logger class name which implements
> >> oracle.toplink.essentials.logging.SessionLog
> >> ! // or one of values defined in LoggerType.
> >> ! public static final String LOGGING_LOGGER =
> >> "toplink.logging.logger";
> >> // Valid values are names of levels defined in
> >> java.util.logging.Level,
> >> // default value is java.util.logging.Level.CONFIG.getName()
> >> public static final String LOGGING_LEVEL = "toplink.logging.level
> ";
> >> Index:
> >>
> src/java/oracle/toplink/essentials/exceptions/EntityManagerSetupException.java
> >>
> >> ===================================================================
> >> RCS file:
> >>
> /cvs/glassfish/entity-persistence/src/java/oracle/toplink/essentials/exceptions/EntityManagerSetupException.java,v
> >>
> >> retrieving revision 1.5
> >> diff -c -r1.5 EntityManagerSetupException.java
> >> ***
> >>
> src/java/oracle/toplink/essentials/exceptions/EntityManagerSetupException.java
> >> 11 May 2006 19:30:21 -0000 1.5
> >> ---
> >>
> src/java/oracle/toplink/essentials/exceptions/EntityManagerSetupException.java
> >> 12 Sep 2006 05:48:49 -0000
> >> ***************
> >> *** 38,43 ****
> >> --- 38,44 ----
> >> public static final int WRONG_PROPERTY_VALUE_TYPE = 28012;
> >> public static final int CANNOT_DEPLOY_WITHOUT_PREDEPLOY = 28013;
> >> public static final int FAILED_WHILE_PROCESSING_PROPERTY = 28014;
> >> + public static final int FAILED_TO_INSTANTIATE_LOGGER = 28015;
> >>
> >> /**
> >> ***************
> >> *** 177,180 ****
> >> --- 178,188 ----
> >> return setupException;
> >> }
> >>
> >> + public static EntityManagerSetupException
> >> failedToInstantiateLogger(String loggerClassName, String propertyName,
> >> Exception exception) {
> >> + Object[] args = { loggerClassName, propertyName };
> >> +
> >> + EntityManagerSetupException setupException = new
> >> EntityManagerSetupException(ExceptionMessageGenerator.buildMessage
> >> (EntityManagerSetupException.class, FAILED_TO_INSTANTIATE_LOGGER,
> >> args), exception);
> >> + setupException.setErrorCode (FAILED_TO_INSTANTIATE_LOGGER);
> >> + return setupException;
> >> + }
> >> }
> >> Index:
> >>
> src/java/oracle/toplink/essentials/exceptions/i18n/EntityManagerSetupExceptionResource.java
> >>
> >> ===================================================================
> >> RCS file:
> >>
> /cvs/glassfish/entity-persistence/src/java/oracle/toplink/essentials/exceptions/i18n/EntityManagerSetupExceptionResource.java,v
> >>
> >> retrieving revision 1.8
> >> diff -c -r1.8 EntityManagerSetupExceptionResource.java
> >> ***
> >>
> src/java/oracle/toplink/essentials/exceptions/i18n/EntityManagerSetupExceptionResource.java
> >> 8 Sep 2006 18:53:05 -0000 1.8
> >> ---
> >>
> src/java/oracle/toplink/essentials/exceptions/i18n/EntityManagerSetupExceptionResource.java
> >> 12 Sep 2006 05:48:49 -0000
> >> ***************
> >> *** 44,50 ****
> >> { "28011", "The session,
> >> [{0}], built for a persistence unit was not available at the time it
> >> was deployed. This means that somehow the session was removed from
> >> the container in the middle of the deployment process." },
> >> { "28012", "Value [{0}]
> >> is of incorrect type for property [{2}], value type should be [{1}]."
> },
> >> { "28013", "Attempted to
> >> deploy PersistenceUnit [{0}] for which predeploy method either had not
> >> called or had failed" },
> >> ! { "28014", "Exception was
> >> thrown while processing property [{0}] with value [{1}]." }
> >> };
> >>
> >> /**
> >> --- 44,51 ----
> >> { "28011", "The session,
> >> [{0}], built for a persistence unit was not available at the time it
> >> was deployed. This means that somehow the session was removed from
> >> the container in the middle of the deployment process." },
> >> { "28012", "Value [{0}]
> >> is of incorrect type for property [{2}], value type should be [{1}]."
> },
> >> { "28013", "Attempted to
> >> deploy PersistenceUnit [{0}] for which predeploy method either had not
> >> called or had failed" },
> >> ! { "28014", "Exception was
> >> thrown while processing property [{0}] with value {[1}]." },
> >> ! { "28015", "Failed to
> >> instantiate SessionLog of type [{0}] specified in [{1}] property." }
> >> };
> >>
> >> /**
> >> Index:
> >>
> src/java/oracle/toplink/essentials/internal/ejb/cmp3/EntityManagerSetupImpl.java
> >>
> >> ===================================================================
> >> RCS file:
> >>
> /cvs/glassfish/entity-persistence/src/java/oracle/toplink/essentials/internal/ejb/cmp3/EntityManagerSetupImpl.java,v
> >>
> >> retrieving revision 1.40
> >> diff -c -r1.40 EntityManagerSetupImpl.java
> >> ***
> >>
> src/java/oracle/toplink/essentials/internal/ejb/cmp3/EntityManagerSetupImpl.java
> >> 7 Sep 2006 19:54:31 -0000 1.40
> >> ---
> >>
> src/java/oracle/toplink/essentials/internal/ejb/cmp3/EntityManagerSetupImpl.java
> >> 12 Sep 2006 05:48:50 -0000
> >> ***************
> >> *** 331,336 ****
> >> --- 331,357 ----
> >> return serverPlatform;
> >> }
> >> +
> >> + /**
> >> + * INTERNAL:
> >> + * Build a SessionLog class instance from the logger type
> property.
> >> + */
> >> + protected SessionLog getSessionLog(Map m) {
> >> + String loggerClassName =
> >> (String)PropertiesHandler.getPropertyValueLogDebug(
> >> TopLinkProperties.LOGGING_LOGGER, m, session);
> >> + if (loggerClassName == null) {
> >> + return null;
> >> + }
> >> +
> >> + SessionLog sessionLog = null;
> >> + Class cls = findClassForProperty(loggerClassName,
> >> TopLinkProperties.LOGGING_LOGGER);
> >> + try {
> >> + sessionLog = (SessionLog)cls.newInstance();
> >> + } catch (Exception ex) {
> >> + throw
> >> EntityManagerSetupException.failedToInstantiateLogger
> >> (loggerClassName, TopLinkProperties.LOGGING_LOGGER, ex);
> >> + }
> >> + return sessionLog;
> >> +
> >> + }
> >> protected static Class findClass(String className) throws
> >> ClassNotFoundException, PrivilegedActionException {
> >> if (PrivilegedAccessHelper.shouldUsePrivilegedAccess()){
> >> ***************
> >> *** 460,467 ****
> >> session = new ServerSession(new Project(new
> DatabaseLogin()));
> >>
> >> ServerPlatform serverPlatform =
> >> getServerPlatform(predeployProperties);
> >> ! if (serverPlatform != null){
> >> ! AbstractSessionLog.setLog(serverPlatform.getServerLog());
> >> }
> >>
> >> //Bug5389828. Update the logging settings for the singleton
> >> logger.
> >> --- 481,494 ----
> >> session = new ServerSession(new Project(new
> DatabaseLogin()));
> >>
> >> ServerPlatform serverPlatform =
> >> getServerPlatform(predeployProperties);
> >> ! // SessionLog can be specified by the logger property or
> >> ServerPlatform.getServerLog().
> >> ! // The logger property has a higher priority to
> >> ServerPlatform.getServerLog().
> >> ! SessionLog sessionLog = getSessionLog(predeployProperties);
> >> ! if (sessionLog == null && serverPlatform != null){
> >> ! sessionLog = serverPlatform.getServerLog();
> >> ! }
> >> ! if (sessionLog != null){
> >> ! AbstractSessionLog.setLog (sessionLog);
> >> }
> >>
> >> //Bug5389828. Update the logging settings for the singleton
> >> logger.
> >> ***************
> >> *** 469,478 ****
> >> // build a list of entities the persistence unit represented
> >> by this EntityManagerSetupImpl will use
> >> Collection entities = buildEntityList(persistenceUnitInfo,
> >> privateClassLoader);
> >>
> >> if (serverPlatform != null){
> >> - session.setSessionLog (serverPlatform.getServerLog());
> >> session.setServerPlatform(serverPlatform);
> >> ! }
> >>
> >> initOrUpdateLogging(true, predeployProperties,
> >> session.getSessionLog());
> >>
> >> EntityManagerFactoryProvider.warnOldProperties(predeployProperties,
> >> session);
> >> --- 496,507 ----
> >> // build a list of entities the persistence unit represented
> >> by this EntityManagerSetupImpl will use
> >> Collection entities = buildEntityList(persistenceUnitInfo,
> >> privateClassLoader);
> >>
> >> + if (sessionLog != null){
> >> + session.setSessionLog (sessionLog);
> >> + }
> >> if (serverPlatform != null){
> >> session.setServerPlatform(serverPlatform);
> >> ! }
> >>
> >> initOrUpdateLogging(true, predeployProperties,
> >> session.getSessionLog());
> >>
> >> EntityManagerFactoryProvider.warnOldProperties(predeployProperties,
> >> session);
> >> Index:
> >>
> src/java/oracle/toplink/essentials/internal/ejb/cmp3/base/PropertiesHandler.java
> >>
> >> ===================================================================
> >> RCS file:
> >>
> /cvs/glassfish/entity-persistence/src/java/oracle/toplink/essentials/internal/ejb/cmp3/base/PropertiesHandler.java,v
> >>
> >> retrieving revision 1.2
> >> diff -c -r1.2 PropertiesHandler.java
> >> ***
> >>
> src/java/oracle/toplink/essentials/internal/ejb/cmp3/base/PropertiesHandler.java
> >> 31 Aug 2006 02:58:14 -0000 1.2
> >> ---
> >>
> src/java/oracle/toplink/essentials/internal/ejb/cmp3/base/PropertiesHandler.java
> >> 12 Sep 2006 05:48:50 -0000
> >> ***************
> >> *** 147,152 ****
> >> --- 147,153 ----
> >> boolean shouldReturnOriginalValueIfValueToApplyNotFound;
> >> static {
> >> + addProp(new LoggerTypeProp());
> >> addProp(new LoggingLevelProp());
> >> addProp(new TargetDatabaseProp());
> >> addProp(new TargetServerProp());
> >> ***************
> >> *** 374,379 ****
> >> --- 375,392 ----
> >> static void addProp(Prop prop) {
> >> prop.initialize();
> >> mainMap.put(prop.name <http://prop.name>, prop);
> >> + }
> >> + }
> >> +
> >> + protected static class LoggerTypeProp extends Prop {
> >> + LoggerTypeProp() {
> >> + super(TopLinkProperties.LOGGING_LOGGER,
> >> LoggerType.DEFAULT);
> >> + this.shouldReturnOriginalValueIfValueToApplyNotFound =
> >> true;
> >> + String pcg = "oracle.toplink.essentials.logging.";
> >> + valueArray = new Object[][] {
> >> + {LoggerType.DefaultLogger , pcg +
> "DefaultSessionLog"},
> >> + {LoggerType.JavaLogger, pcg + "JavaLog"}
> >> + };
> >> }
> >> }
> >>
> >> Thanks,
> >> -Wonseok
> >>
> >> On 9/12/06, *Marina Vatkina * <Marina.Vatkina_at_sun.com
> >> <mailto:Marina.Vatkina_at_sun.com>> wrote:
> >>
> >> Sounds good to me.
> >>
> >> thanks,
> >> -marina
> >>
> >> Wonseok Kim wrote On 09/11/06 09:19,:
> >> > Modified following lines as Tom suggested.
> >> >
> >> >> This works similar to ServerPlatform.getSessionLog ().
> >> >> This property is only used if there is no ServerPlatform.
> >> >> If "toplink.target-server" property is specified, then the
> >> specified
> >> >> ServerPlatform's getSessionLog()
> >> >> is used to change the logger type and " toplink.logging.logger"
> >> will be
> >> >> ignored(warning is required?).
> >> >
> >> > Modified:
> >> > This works similar to ServerPlatform.getSessionLog(), but this
> >> logger
> >> > type property has a higher priority to
> >> ServerPlatform.getSessionLog ().
> >> > It means that if server-platform("toplink.target-server" or its
> >> > deprecated one) property is specified explicitly by user or
> >> impliciltly
> >> > by application server such as GlassFish,
> >> ServerPlatform.getSessionLog ()
> >> > is not used and the specified logger type is used.
> >> >
> >> > One more thought. I obtain Valid values from physical SessionLog
> >> class
> >> > names - DefaultSessionLog, JavaLog. How about using more logical
> >> value?
> >> > "DefaultLogger", "JavaLogger"? Physical class name might be
> >> changed in
> >> > the future and logical one is easy to remember.
> >> >
> >> > Thanks
> >> > -Wonseok
> >> >
> >> >
> >> > On 9/11/06, *Tom Ware* < tom.ware_at_oracle.com
> >> <mailto:tom.ware_at_oracle.com>
> >> > <mailto:tom.ware_at_oracle.com <mailto:tom.ware_at_oracle.com>>> wrote:
> >> >
> >> > Hi Wonseok,
> >> >
> >> > I think this is a good proposal. My only suggestion is
> >> that we might
> >> > want to have the specified logger override the one specified
> >> in the
> >> > server platform.
> >> >
> >> > -Tom
> >> >
> >> > Wonseok Kim wrote:
> >> >
> >> > > Hi, Tom, Marina
> >> > >
> >> > > May I propose this feature? This is simple to solve, so I
> >> have just
> >> > > implemented the prototype and worked fine.
> >> > >
> >> > > Property name: toplink.logging.logger
> >> > > Description: Specify the type of logger.
> >> > > Purpose: Let's make it easy to change the logger type in
> >> Java SE mode.
> >> > > Valid values: logger class name which implements
> >> > > oracle.toplink.essentials.logging.SessionLog
> >> > > or one of the following string values.
> >> > > * DefaultSessionLog (default)
> >> > > * JavaLog
> >> > >
> >> > > Example: persistence.xml file
> >> > > <property name="toplink.logging.logger" value="JavaLog"/>
> >> > >
> >> > > This works similar to ServerPlatform.getSessionLog ().
> >> > > This property is only used if there is no ServerPlatform.
> >> > > If "toplink.target-server" property is specified, then the
> >> specified
> >> > > ServerPlatform's getSessionLog()
> >> > > is used to change the logger type and
> >> "toplink.logging.logger " will be
> >> > > ignored(warning is required?).
> >> > >
> >> > > Currently there are just two logger types - default and
> >> Java logger...
> >> > > Maybe another logger type can be added in the future.(such
> >> as log4j)
> >> > >
> >> > > The property name and valid values can be changed if there
> >> are better
> >> > > ones.
> >> > > This is just a proposal and comments are welcome... What
> >> do you think?
> >> > >
> >> > > Thanks
> >> > > - Wonseok
> >> > >
> >> > > On 9/6/06, *Wonseok Kim* <guruwons_at_gmail.com
> >> <mailto:guruwons_at_gmail.com>
> >> > <mailto: guruwons_at_gmail.com <mailto:guruwons_at_gmail.com>>
> >> > > <mailto:guruwons_at_gmail.com <mailto:guruwons_at_gmail.com>
> >> <mailto:guruwons_at_gmail.com <mailto:guruwons_at_gmail.com>>>> wrote:
> >> > >
> >> > > I filed an issue for this.
> >> > >
> >> https://glassfish.dev.java.net/issues/show_bug.cgi?id=1096
> >> > > <
> >> https://glassfish.dev.java.net/issues/show_bug.cgi?id=1096>
> >> > >
> >> > > Also I wrote a blog with regard to the workaround Tom
> >> suggested
> >> > > (see issue comment).
> >> > > I couldn't extend NoServerPlatform because it's final
> >> class, so I
> >> > > extended ServerPlatformBase.
> >> > >
> >> > > Thanks
> >> > > - Wonseok
> >> > >
> >> > >
> >> > > On 9/5/06, *Tom Ware* < tom.ware_at_oracle.com
> >> <mailto:tom.ware_at_oracle.com>
> >> > <mailto: tom.ware_at_oracle.com <mailto:tom.ware_at_oracle.com>>
> >> > > <mailto: tom.ware_at_oracle.com
> >> <mailto:tom.ware_at_oracle.com> <mailto:tom.ware_at_oracle.com
> >> <mailto:tom.ware_at_oracle.com>>>> wrote:
> >> > >
> >> > > Hi Wonseok,
> >> > >
> >> > > You are correct, there is no current
> >> configuration option
> >> > > that lets
> >> > > you automatically change the logger. That would
> >> be a good
> >> > feature
> >> > > request to enter in the issue tracker.
> >> > >
> >> > > I believe your best bet is to subclass
> >> NoServerPlatform and
> >> > > override
> >> > > the getServerLog() method in the same way as it is
> >> > implemented in
> >> > > SunAS9ServerPlatform.
> >> > >
> >> > > -Tom
> >> > >
> >> > > Wonseok Kim wrote:
> >> > >
> >> > >> Hi, all
> >> > >>
> >> > >> After I see the discussion thread about issue#988 -
> Logging
> >> > >> integration with GF happens too late,
> >> > >> I got a question about logger in Java SE mode.
> >> > >> Java logger (JavaLog) can be used by configuring
> >> > > ServerPlatform (like
> >> > >> GlassFish), but can it be used in Java SE also?
> >> > >> I can't see any configuration for changing logger - only
> >> > >> DefaultSessionLog is used.
> >> > >>
> >> > >> If it cannot be changed in current implementation, how
> about
> >> > > making
> >> > >> the property like "
> >> toplink.logging.logger=<logger_classname>"?
> >> > >> I think it can be also used in Java EE environment. Then
> it
> >> > > doesn't
> >> > >> need to implement ServerPlatform class to change only
> >> logger
> >> > > in other
> >> > >> Java EE container environment.
> >> > >>
> >> > >> Thanks
> >> > >> --
> >> > >> Wonseok Kim
> >> > >> Senior Developer, TmaxSoft
> >> > >
> >> > >
> >> > >
> >> >
> >> >
> >>
> >>
> >
>
--
Wonseok Kim
Senior Developer, TmaxSoft