Hi Wonseok,
Yes, you are right about #1 and #3. Now #2 needs more discussion...
In this particular case, session doesn't change, only the name, so it
should be OK (I can add more comments), but in general, is there a
possibility that createEMF - emf.close - createEMF will result in a
logger that references an old session? I can't find a code path that
will result in this situation, but I might be wrong.
What do you think?
thanks,
-marina
Wonseok Kim wrote:
> 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
>
>