Looks good to me.
Thanks,
Andrei
----- Original Message -----
From: Wonseok Kim
To: persistence_at_glassfish.dev.java.net
Sent: Monday, November 06, 2006 8:53 PM
Subject: Re: Final review: adding server platform and session name change into logger integration
Hi Marina,
Looks good to me and looks simpler than the previous ones.
Cheers
-Wonseok
On 11/7/06, Marina Vatkina < Marina.Vatkina_at_sun.com> wrote:
Andrei, Wonseok,
The (hopefully) final version is attached. Please let me know
if you have any other concerns.
thanks,
-marina
Andrei Ilitchev wrote On 11/03/06 22:17,:
> Marina,
> ----- Original Message -----
> From: "Marina Vatkina" < Marina.Vatkina_at_Sun.COM>
> To: <persistence_at_glassfish.dev.java.net>
> Sent: Friday, November 03, 2006 7:19 PM
> Subject: Re: Reminder: adding server platform and session name change into
> logger integration
>
>
>
>>Andrei, Wonseok,
>>
>>Today's version is attached.
>>
>>Andrei, it assumes your fix in NoServerPlatform, but still relies on the
>>return value from update... methods because otherwise the code will be
>>creating new instances of the loggers without real need for it (e.g. in
>>case
>>of a session name change when either loggerClass or serverPlatform has
>>changed
>>as well, or when comparing an existing logger class with the one provided
>>by
>>the platform). There was also a typo in your version, where you call
>>setServerSessionName instead of updateSessionName in updateServerSession.
>
> Alright.
> Just change in updateLoggers:
> if (loggerClassName != null &&
> !currentLog.getClass().getName().equals(loggerClassName)) {
> ...
> } else if(serverPlatformChanged) {
> ServerPlatform serverPlatform = session.getServerPlatform();
> singletonLog = serverPlatform.getServerLog();
> sessionLog = serverPlatform.getServerLog();
> }
> to:
> if (loggerClassName != null) {
> if(!currentLog.getClass().getName().equals(loggerClassName)) {
> ....
> }
> } else if(serverPlatformChanged) {
> ServerPlatform serverPlatform = session.getServerPlatform();
> singletonLog = serverPlatform.getServerLog();
> sessionLog = serverPlatform.getServerLog();
> }
>
> If loggerClassName is provided we shouldn't go to serverPlatform just
> because the provided class happens to be the same as the current one.
>
>
>>One more question for Tom and Andrei - predeployProperties is an instance
>>variable and it's some times used as-is and some times is passed around
>>as a parameter to other methods. Which one is correct?
>
> Both:
> some methods always use predeployProperies,
> others, like updateLoggers called several times with various maps.
>
>
>>As usual, I tested with in-container logging behavior, and e-t-p tests.
>>
>>thanks,
>>-marina
>>
>>Andrei Ilitchev wrote:
>>
>>>Marina, Wonseok,
>>>
>>>Attached is my version of EntityManagerSetupImpl class - I hope it's
>>>more simple.
>>>
>>>Also attached is a new version of NoServerPlatform class - the old one
>>>broke the pattern of creating a new instance of Log in getServer method.
>>>
>>>Thanks,
>>>
>>>Andrei
>>>
>>>----- Original Message ----- From: "Marina Vatkina"
>>><Marina.Vatkina_at_Sun.COM>
>>>To: <persistence_at_glassfish.dev.java.net>
>>>Sent: Friday, November 03, 2006 1:44 PM
>>>Subject: Reminder: adding server platform and session name change into
>>>logger integration
>>>
>>>
>>>
>>>>Tom, Andrei,
>>>>
>>>>Do you have any comments, or should I incorporate this last change
>>>>and check in the code? Will anybody want to see the final code, if
>>>>this is the only change?
>>>>
>>>>thanks,
>>>>-marina
>>>>
>>>>Marina Vatkina wrote:
>>>>
>>>>
>>>>>Hi Wonseok,
>>>>>
>>>>>Thanks for looking into this. Yeah, I was also thinking about it.
>>>>>It might even remove the need for the 'init' param - it's only
>>>>>used to bypass updateSessionName() call. So it'll be like
>>>>> updateLoggers(m, serverPlatformChanged, false);
>>>>>and
>>>>> updateLoggers(m, serverPlatformChanged, sessionNameChanged);
>>>>>
>>>>>I'll wait for Tom and Andrei to see if they have any other comments,
>>>>>before making further changes.
>>>>>
>>>>>thanks,
>>>>>-marina
>>>>>
>>>>>Wonseok Kim wrote:
>>>>>
>>>>>
>>>>>>Marina,
>>>>>>Thanks for solving the puzzle of the logger class change!
>>>>>>
>>>>>>It looks good except for one thing.
>>>>>>
>>>>>>Now updateServerPlatform() and updateSessionName() are done in
>>>>>>updateLoggers(), but doesn't it look a little bit awkward?
>>>>>>updateLoggers() just need to know whether ServerPlatform and session
>>>>>>name change. It would be easy to understand the code intuitively if
>>>>>>updateServerPlatform() and updateSessionName() are called in
>>>>>>predeploy() and updateServerSession() like below.
>>>>>>
>>>>>> protected void updateServerSession(Map m) {
>>>>>> ...
>>>>>> // In deploy Session name and ServerPlatform could've
>>>>>>changed which will affect the loggers.
>>>>>> boolean serverPlatformChanged = updateServerPlatform(m);
>>>>>> boolean sessionNameChanged = updateSessionName(m);
>>>>>> updateLoggers(m, false, serverPlatformChanged,
>>>>>>sessionNameChanged);
>>>>>>
>>>>>>Cheers,
>>>>>>-Wonseok
>>>>>>
>>>>>>On 11/3/06, * Marina Vatkina* <Marina.Vatkina_at_sun.com
>>>>>><mailto: Marina.Vatkina_at_sun.com>> wrote:
>>>>>>
>>>>>> Team,
>>>>>>
>>>>>> One more attempt to solve this puzzle is attached. Again, tested
>>>>>>in the
>>>>>> container and via e-p-t tests (i.e. no special testing for SE
>>>>>>changes of
>>>>>> the platform, session name, and logging).
>>>>>>
>>>>>> Please review *very carefully* ;).
>>>>>>
>>>>>> thanks,
>>>>>> -marina
>>>>>>
>>>>>> Marina Vatkina wrote:
>>>>>> > Hi Wonseok,
>>>>>> >
>>>>>> > Wonseok Kim wrote:
>>>>>> >
>>>>>> >> Hello Marina,
>>>>>> >>
>>>>>> >> I realized today that logger property(toplink.logging.logger
>>>>>>) can
>>>>>> >> change also in deploy phase like other properties.
>>>>>> >> If it is different, loggers should be updated also. Do you
>>>>>>want
>>>>>> to fix
>>>>>> >> it also or file it as another issue?
>>>>>> >
>>>>>> >
>>>>>> > Probably. Let me think about it.
>>>>>> >
>>>>>> >>
>>>>>> >> I found also that your modification is not using member
>>>>>>variable
>>>>>> >> "serverPlatform" anymore which you introduced before.
>>>>>> >
>>>>>> >
>>>>>> > Thanks.
>>>>>> >
>>>>>> >> As to session.setSessionLog (), just do as you wish, I think
>>>>>>it's
>>>>>> not a
>>>>>> >> big deal.
>>>>>> >
>>>>>> >
>>>>>> > OK.
>>>>>> >
>>>>>> > thanks,
>>>>>> > -marina
>>>>>> >
>>>>>> >>
>>>>>> >> Cheers,
>>>>>> >> -Wonseok
>>>>>> >>
>>>>>>
>>>>>>
>