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
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>
>>>>>
>>>>>
>>>
>>>
>
>