persistence@glassfish.java.net

Re: adding server platform and session name change into logger integration

From: Wonseok Kim <guruwons_at_gmail.com>
Date: Fri, 3 Nov 2006 11:12:06 +0900

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