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
>
> On 11/2/06, *Marina Vatkina* <Marina.Vatkina_at_sun.com
> <mailto:Marina.Vatkina_at_sun.com>> wrote:
>
> Andrei, Tom, Wonseok,
>
> Attached is the updated version. I tested the logging part only
> in the container so far (unit tests passed, but I didn't test
> specifically the logging behavior - what can I be looking for?).
>
> Wonseok, I didn't change code in AbstractSessionLog.setSession()
> as we should not be changing the session at any time, but added
> a comment when we call the setSession() method.
>
> I can also change
> AbstractSessionLog.setLog(new DefaultSessionLog());
> session.setSessionLog(new DefaultSessionLog());
>
> to
> AbstractSessionLog.setLog(new DefaultSessionLog());
> session.setSessionLog (AbstractSessionLog.getLog());
>
> to use cloning inside session's method (I don't see anything in
> the DefaultSessionLog, that can cause a problem in this case).
> On the other hand, the current code doesn't need extra explanation ;).
>
> thanks,
> -marina
>
> Andrei Ilitchev wrote On 11/01/06 07:12,:
> > Marina,
> >
> > Session's constructor sets serverPlatform to NoServerPlatform -
> don't check
> > for null, check for NoServerPlatform.
> > ----- Original Message -----
> > From: "Marina Vatkina" <Marina.Vatkina_at_Sun.COM
> <mailto:Marina.Vatkina_at_Sun.COM>>
> > To: < persistence_at_glassfish.dev.java.net
> <mailto:persistence_at_glassfish.dev.java.net>>
> > Sent: Tuesday, October 31, 2006 6:15 PM
> > Subject: Re: adding server platform and session name change into
> logger
> > integration
> >
> >
> >
> >>Which makes it even more complicated :(.
> >>
> >>The current code in the logging (no, you can't ignore it when you
> touch
> >>the server platform or the session name) relies at 'null' as the
> "default"
> >>logger use case. Will it be safe to check 'instanceof
> NoServerPlatform' in
> >>addition to null?
> >>
> >>thanks,
> >>-marina
> >>
> >>Andrei Ilitchev wrote:
> >>
> >>>Marina,
> >>>
> >>>PropertyHandler returns default value (which is
> NoServerPlatform) in case
> >>>an property value is an empty string.
> >>>So to get back NoServerPlatform, the user should set the property to
> >>>either an empty string, or "None", or
> >>>" oracle.toplink.essentials.platform.server.NoServerPlatform.
> >>>
> >>>
> >>>----- Original Message ----- From: "Marina Vatkina"
> >>>< Marina.Vatkina_at_Sun.COM <mailto:Marina.Vatkina_at_Sun.COM>>
> >>>To: <persistence_at_glassfish.dev.java.net
> <mailto:persistence_at_glassfish.dev.java.net>>
> >>>Sent: Tuesday, October 31, 2006 4:00 PM
> >>>Subject: Re: adding server platform and session name change into
> logger
> >>>integration
> >>>
> >>>
> >>>
> >>>>Hi Andrei,
> >>>>
> >>>>Yes, this is better. But I'm confused... You filed an issue
> 1333 that
> >>>>server
> >>>>platform can be unset in the map. How will it be specified by
> the user?
> >>>>If
> >>>>a) as an empty string - your code doesn't handle this case.
> >>>>b) as "NoServerPlatform" - will you use reflection to create it?
> >>>>
> >>>>thanks,
> >>>>-marina
> >>>>
> >>>>Andrei Ilitchev wrote:
> >>>>
> >>>>
> >>>>>Let's try another file type...
> >>>>>----- Original Message ----- From: "Marina Vatkina"
> >>>>><Marina.Vatkina_at_Sun.COM <mailto:Marina.Vatkina_at_Sun.COM>>
> >>>>>To: < persistence_at_glassfish.dev.java.net
> <mailto:persistence_at_glassfish.dev.java.net>>
> >>>>>Sent: Tuesday, October 31, 2006 2:28 PM
> >>>>>Subject: Re: adding server platform and session name change
> into logger
> >>>>>integration
> >>>>>
> >>>>>
> >>>>>
> >>>>>>Andrei,
> >>>>>>
> >>>>>>Can you please resend the file? It came as 0 size.
> >>>>>>
> >>>>>>thanks,
> >>>>>>-marina
> >>>>>>
> >>>>>>Andrei Ilitchev wrote:
> >>>>>>
> >>>>>>
> >>>>>>>Hi Marina, Wonseok,
> >>>>>>> Attached is my version of changes to EntityManagerSetupImpl
> class.
> >>>>>>> I concentrated on handling of serverPlatform property and
> didn't
> >>>>>>>look at the logging stuff.
> >>>>>>> Thanks,
> >>>>>>> Andrei
> >>>>>>>
> >>>>>>> ----- Original Message -----
> >>>>>>> *From:* Wonseok Kim <mailto: guruwons_at_gmail.com
> <mailto:guruwons_at_gmail.com>>
> >>>>>>> *To:* persistence_at_glassfish.dev.java.net
> <mailto:persistence_at_glassfish.dev.java.net>
> >>>>>>> <mailto: persistence_at_glassfish.dev.java.net
> <mailto:persistence_at_glassfish.dev.java.net>>
> >>>>>>> *Sent:* Tuesday, October 31, 2006 11:20 AM
> >>>>>>> *Subject:* Re: adding server platform and session name
> change
> >>>>>>>into
> >>>>>>> logger integration
> >>>>>>>
> >>>>>>> Hi Marina,
> >>>>>>> Overall, it looks good to me.
> >>>>>>>
> >>>>>>> Some minor comments below.
> >>>>>>>
> >>>>>>> protected void updateServerSession(Map m) {
> >>>>>>> if (session == null || session.isConnected()) {
> >>>>>>> return;
> >>>>>>> }
> >>>>>>>
> >>>>>>> // Session name and ServerPlatform could've
> changed which
> >>>>>>> will affect the loggers.
> >>>>>>> boolean serverPlatformChanged =
> >>>>>>>setOrUpdateServerPlatform(m);
> >>>>>>> boolean sessionNameChanged = updateSessionName(m);
> >>>>>>>
> >>>>>>> if (serverPlatformChanged) {
> >>>>>>> // Update loggers and settings for the singleton
> >>>>>>>logger
> >>>>>>> and the session logger.
> >>>>>>> updateLoggers(predeployProperties);
> >>>>>>> } else if (sessionNameChanged) {
> >>>>>>> // In JavaLog this will result in logger name
> changes,
> >>>>>>> // but won't affect DefaultSessionLog.
> >>>>>>> session.getSessionLog().setSession(session);
> >>>>>>> }
> >>>>>>>
> >>>>>>> -> you mean updateLoggers(m), right?
> >>>>>>>
> >>>>>>> AbstractSessionLog.setSession() need to be changed to
> accept new
> >>>>>>> session, see below.
> >>>>>>>
> >>>>>>> public void setSession(Session session) {
> >>>>>>> if (this.session == null) {
> >>>>>>> this.session = session;
> >>>>>>> buildSessionType();
> >>>>>>> buildSessionHashCode();
> >>>>>>> }
> >>>>>>> }
> >>>>>>>
> >>>>>>> initOrUpdateLogging() doesn't need 'init' parameter
> anymore after
> >>>>>>> your fix for issue 1107.
> >>>>>>>
> >>>>>>> protected void initOrUpdateLogging(boolean init, Map m,
> >>>>>>> SessionLog log) {
> >>>>>>>
> >>>>>>> Cheers,
> >>>>>>> -Wonseok
> >>>>>>>
> >>>>>>>
> >>>>>>> On 10/31/06, *Marina Vatkina* <Marina.Vatkina_at_sun.com
> <mailto:Marina.Vatkina_at_sun.com>
> >>>>>>> <mailto:Marina.Vatkina_at_sun.com
> <mailto:Marina.Vatkina_at_sun.com>>> wrote:
> >>>>>>>
> >>>>>>> Tom, Andrei, Wonseok,
> >>>>>>>
> >>>>>>> Attached is the changed file with the ideas for
> fixing this
> >>>>>>> problem. I
> >>>>>>> didn't test it yet, but would like to start the
> discussion as
> >>>>>>> the practice
> >>>>>>> shows that changing EMSetupImpl is a painful exercise.
> >>>>>>>
> >>>>>>> Probably the main change for you guys is replacing
> void with
> >>>>>>>a
> >>>>>>> boolean
> >>>>>>> as the returning type for updateSessionName().
> Please do not
> >>>>>>> panic, but
> >>>>>>> suggest your ideas if you don't like it - I need to
> know if
> >>>>>>>the
> >>>>>>> name has
> >>>>>>> changed and would like to avoid saving the name, then
> >>>>>>>comparing
> >>>>>>> it again
> >>>>>>> after it had been replaced or not: the comparison is
> done
> >>>>>>>inside
> >>>>>>> the method
> >>>>>>> anyway. Keep in mind that if the server platform has
> changed,
> >>>>>>>I
> >>>>>>> need the
> >>>>>>> new session name before fixing the loggers.
> >>>>>>>
> >>>>>>> As you can see, in case of the server platform
> change, all of
> >>>>>>> the loggers
> >>>>>>> need to be replaced (we can probably go further and
> check the
> >>>>>>> actual logger
> >>>>>>> type change, but I don't think it's worth the
> effort), in
> >>>>>>> particular when
> >>>>>>> the value is unset in the Map.
> >>>>>>>
> >>>>>>> If only the session name change, it seems to be
> enough to
> >>>>>>>update
> >>>>>>> the session
> >>>>>>> in the existing logger (and do not touch the default
> logger
> >>>>>>>at all).
> >>>>>>>
> >>>>>>> Wonseok, that also meant moving
> initOrUpdateLogging() outside
> >>>>>>>the
> >>>>>>> updateLoggers() call. I also removed parameters from
> the
> >>>>>>>method
> >>>>>>> signature
> >>>>>>> where they are available as instance variable.
> >>>>>>>
> >>>>>>> If you all agree, I'll start running the tests. If
> not, I'd
> >>>>>>>like
> >>>>>>> to sort
> >>>>>>> everything out before going further.
> >>>>>>>
> >>>>>>> thanks,
> >>>>>>> -marina
> >>>>>>>
> >>>>>>>
> >>>>>>
>
>