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