Hi Marina,
The initial bug was that the logging that occurs in
buildEntityList(persistenceUnitInfo, privateClassLoader) was not
affected by the logging settings.
I am attaching some suggested changes - most are based on your
initial fix. Admittedly, there are a couple of pieces that are somewhat
hacky.
1. The way we deal with the fact that ServerPlatform could come from one
of the deprecated properties
2. Only the non-deprecated properties will affect the logging that
occurs in buildEntityListt(persistenceUnitInfo, privateClassLoader)
These changes are just a suggestion and totally open to alternate
changes, and I have not done much testing, but figured I pass on my
thoughts.
-Tom
Marina Vatkina wrote:
>Hi Tom,
>
>Tom Ware wrote:
>
>
>>Hi Marina,
>>
>> I think there still may be a couple of issues:
>>
>>1. Doesn't updateServerPlatform() replace the logger with the server
>>one?
>>
>>
>
>Yes.
>
> If so, don't we lose the settings from the initOfUpdateLogging()
>
>
>>calls that occur before update updateServerPlatform()?
>>
>>
>
>We propagate the setting in setNewLog:
>
> newLog.setLevel(session.getSessionLog().getLevel());
> newLog.setShouldPrintDate(session.getSessionLog().shouldPrintDate());
> newLog.setShouldPrintThread(session.getSessionLog().shouldPrintThread());
> newLog.setShouldPrintSession(session.getSessionLog().shouldPrintSession());
>
>newLog.setShouldLogExceptionStackTrace(session.getSessionLog().shouldLogExceptionStackTrace());
>
> session.setSessionLog(newLog);
>
>
>But(!) we are doing it anyway even now, just a bit later.
>
> We cannot do
>
>
>>that because it will reopen the bug that initially causes us to change
>>where our logging settings are set.
>>
>>
>
>What was that bug about?
>
>
>
>>2. In my mind, this bug is not fixed until all logging messages go to
>>the correct place. If the default log is not set on AbstractSessionLog,
>>that is not done. Can we not, make a call to
>>AbstractSessionLog.setLog() with the correct log?
>>
>>
>
>Here is the problem:
>EntityManagerFactoryProvider.translateOldProperties needs a logger, and
>we can't get the correct server platform without prior translation (blame
>those who decided to invent new names :().
>
>
>
>
>> As a result of these two concerns, I think there is some work to do.
>>Perhaps a solution could be to break up the updateServerPlatform in to
>>two methods. 1 - get the serverPlatform instance, 2 - set it on the
>>session. If you do that, the first method could be called before the
>>first initOrUpdateLogging call, and the instance derived from it could
>>to set the correct logger both on the session and on the
>>AbstractSesssionLog at the appropriate time.
>>
>>
>
>OK. Let's discuss it further.
>
>
>>-Tom
>>
>>Marina Vatkina wrote:
>>
>>
>>
>>>Hi Tom,
>>>
>>>Tom Ware wrote:
>>>
>>>
>>>
>>>
>>>>Hi Marina,
>>>>
>>>>First of all, sorry for the confusion.
>>>>
>>>>
>>>>
>>>No problem. Good think I asked ;)
>>>
>>>
>>>
>>>
>>>>Now a question: Are we also adding some code that sets the default
>>>>log? I am surprised I do not see any code that influences
>>>>AbstractSessionLog's default log. Don't we need to logging from
>>>>AbstractSessionLog.getLog() to be logged to the correct place?
>>>>
>>>>
>>>>
>>>Yes, I'd like to understand the whole initOrUpdateLogging() story
>>>myself.
>>>
>>>Do you see a problem if I file a separate issue for that and check
>>>in my changes now?
>>>
>>>
>>>
>>>
>>>
>>>
>>>>Aside from that, I think moving the updateServerPlatform call and
>>>>chaning the location of the JTA initialization will be fine.
>>>>
>>>>ServerPlatform should not change in the middle of a run, so I think
>>>>removing the call to this method in updateServerSession should be fine.
>>>>
>>>>
>>>>
>>>OK.
>>>
>>>thanks,
>>>-marina
>>>
>>>
>>>
>>>
>>>
>>>>-Tom
>>>>
>>>>
>>>>Marina Vatkina wrote:
>>>>
>>>>
>>>>
>>>>
>>>>
>>>>>Hi Tom,
>>>>>
>>>>>I think (I don't know why I didn't notice it right away) that there is
>>>>>some disconnect here ;).
>>>>>
>>>>>I didn't change updateServerSession. I added a call to
>>>>>updateServerPlatform
>>>>>from predeploy() *and* modified that (updateServerPlatform) method
>>>>>not to
>>>>>set JTA flag. Setting JTA flag is called explicitly from
>>>>>updateServerSession
>>>>>after updateLogins() call.
>>>>>
>>>>>Now, all updateServerPlatform is doing, is loading the class, then sets
>>>>>session.setServerPlatform(serverPlatform), and sets the log. I can't
>>>>>set
>>>>>the log without loading the platform class. Now,
>>>>>a) how harmful is session.setServerPlatform(serverPlatform) call?
>>>>>b) can server platform change in the middle of the run? If yes, what
>>>>>would
>>>>>it mean? If no, then I can make another change, and completely
>>>>>remove call
>>>>>to this method from updateServerSession.
>>>>>
>>>>>What is wrong with this solution? (Apart that I'm still not sure we
>>>>>are not
>>>>>wasting time on multiple calls to initOrUpdateLogging() - but that
>>>>>can wait).
>>>>>
>>>>>thanks,
>>>>>-marina
>>>>>
>>>>>
>>>>>Tom Ware wrote:
>>>>>
>>>>>
>>>>>
>>>>>
>>>>>
>>>>>
>>>>>>Hi Marina,
>>>>>>
>>>>>>I'll give you a partial explanation and then take an educated guess
>>>>>>at the second part of the answer.
>>>>>>
>>>>>>First, the partial explanation, initOrUpdateLogging() updates the
>>>>>>logging values for a log. There are two logs we are interested in.
>>>>>>
>>>>>>1. The default log
>>>>>>2. The session log
>>>>>>
>>>>>>You will seen each place initOrUpdateLogging() is called, it is
>>>>>>called for each of those logs. I guess that means your update will
>>>>>>not necessarily in that specific method, but it will be where that
>>>>>>method is called.
>>>>>>
>>>>>>Now, the educated guess (the original implementer of this feature
>>>>>>will be available next week to confirm or deny what I say here):
>>>>>>
>>>>>>I believe we make calls to initOrUpdateLogging() both in predeploy
>>>>>>and deploy because we have always had the goal of allowing
>>>>>>application servers to do all the predeployment work ,serialize it
>>>>>>in some way and then do the deployment work later (or over and over
>>>>>>again). When we have implemented a feature like that, making these
>>>>>>calls in both places will make logging more flexible. (i.e. you
>>>>>>will be able to configure it for the predeployment step, and then
>>>>>>later, when you actually deploy you will be able to change the
>>>>>>values.)
>>>>>>
>>>>>>-Tom
>>>>>>
>>>>>>
>>>>>>Marina Vatkina wrote:
>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>>>>Tom,
>>>>>>>
>>>>>>>To change initOrUpdateLogging() I need answers to Q2.2.
>>>>>>>
>>>>>>>thanks,
>>>>>>>-marina
>>>>>>>
>>>>>>>Tom Ware wrote:
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>>Hi Marina,
>>>>>>>>
>>>>>>>>I think we need to investigate solving the logging issue without
>>>>>>>>moving the whole updateServerSession method.
>>>>>>>>
>>>>>>>>The reason I say that is that updateServerSession() occurs in the
>>>>>>>>deploy method by design. We intentionally do not create a server
>>>>>>>>session until deploy is called. (Notice deploy is not called
>>>>>>>>until a ServerSession is required by the EntityManagerFactory)
>>>>>>>>Update ServerSession provides a number of important updates that
>>>>>>>>should only be done when the session is created.
>>>>>>>>
>>>>>>>>Is it possible to make achange in the initOrUpdateLogging() call
>>>>>>>>to first change the default log to the appropriate log prior to
>>>>>>>>all other changes? There is an abstractSessionLog.setLog()
>>>>>>>>method that can be used to set the default session log. A call
>>>>>>>>to that method should result in logging going to the specified log.
>>>>>>>>
>>>>>>>>Here are some answers to a couple of your questions:
>>>>>>>>
>>>>>>>>1. Protected Methods - We realize that in traditional
>>>>>>>>applications only methods that are actually used in a protected
>>>>>>>>manner should be set as protected. Since TopLink is a library
>>>>>>>>that people may want to extend, we provide protected (rather than
>>>>>>>>private) methods as a way of allowing them to do that.
>>>>>>>>2. getServerSession()- It looks like this method can be removed.
>>>>>>>>The ServerSession aquisition logic has been moved to
>>>>>>>>EntityManagerFactoryImpl
>>>>>>>>
>>>>>>>>-Tom
>>>>>>>>
>>>>>>>>Marina Vatkina wrote:
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>>>Team,
>>>>>>>>>
>>>>>>>>>The following solution solves the logger integration problem
>>>>>>>>>(all changes are
>>>>>>>>>made to EntityManagerSetupImpl):
>>>>>>>>>
>>>>>>>>>1. Modify updateServerPlatform() and move the lines
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>if(!session.getLogin().shouldUseExternalTransactionController()) {
>>>>>>>>> serverPlatform.disableJTA();
>>>>>>>>> }
>>>>>>>>>to be after updateLogins(m) as there was a comment
>>>>>>>>>"update ServerPlatform must be called after updateLogins - to
>>>>>>>>>set correct useJTA flag"
>>>>>>>>>
>>>>>>>>>2. Add updateServerPlatform(predeployProperties) after the
>>>>>>>>>second call
>>>>>>>>>to initOrUpdateLogging() in predeploy.
>>>>>>>>>Q2.1: can it be done earlier that that?
>>>>>>>>>Q2.2: Why is this method executed at least 4 times (twice in
>>>>>>>>>predeploy()
>>>>>>>>>plus twice via updateServerSession(), which comments say can happen
>>>>>>>>>more than once)?
>>>>>>>>>
>>>>>>>>>3. I left call to updateServerPlatform() (modified as in #1) from
>>>>>>>>>updateServerSession() as I don't fully understand the reason of
>>>>>>>>>calling it from
>>>>>>>>>there.
>>>>>>>>>Q3.1: Should I just remove it?
>>>>>>>>>
>>>>>>>>>Other questions:
>>>>>>>>>a) Why do we have protected methods in this class that are not
>>>>>>>>>called by any
>>>>>>>>>other class?
>>>>>>>>>
>>>>>>>>>b) What is the purpose of method getServerSession(Map) in
>>>>>>>>>EntityManagerSetupImpl
>>>>>>>>>that is not called by any other code? Can I remove it (I
>>>>>>>>>commented it out and
>>>>>>>>>clean compile had no complains)?
>>>>>>>>>
>>>>>>>>>The changed class is attached (with commented out
>>>>>>>>>getServerSession(Map) method).
>>>>>>>>>
>>>>>>>>>thanks,
>>>>>>>>>-marina
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>
>>>>
>>>>
>>
>>