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?
I found also that your modification is not using member variable
"serverPlatform" anymore which you introduced before.
As to session.setSessionLog(), just do as you wish, I think it's not a big
deal.
Cheers,
-Wonseok
On 11/2/06, Marina Vatkina <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>
> > To: <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>
> >>>To: <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>
> >>>>>To: <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>
> >>>>>>> *To:* 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>> 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
> >>>>>>>
> >>>>>>>
> >>>>>>
>
>