Hi Tom,
No, this doesn't work as PropertiesHandler doesn't know about the old
(deprecated) name and returns null as for unknown property. Should I now
add the old name to the PropertiesHandler as well? This seems like a wrong
idea for handling something that you want to deprecate...
thanks,
-marina
Tom Ware wrote:
> 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
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>
>>>>>
>>>>>
>>>
>>>