Bug 327428

Comment 1 Caspar D. 2010-10-11 06:48:42 EDT

Created attachment 180590 [details]
Patch

Patch fixes the problem and enhances the example to demonstrate
that the failover session and it's TX are actually usable.

Comment 2 Eike Stepper 2010-10-13 02:55:14 EDT

Regarding your question in:

  @Override
  protected void configureSession(InternalCDOSession session)
  {
    super.configureSession(session);

    FailoverCDOSessionImpl sessionImpl = (FailoverCDOSessionImpl)session;
    sessionImpl.setMonitorConnectionDescription(monitorConnectorDescription);
    sessionImpl.setRepositoryGroup(repositoryGroup);
    sessionImpl.setContainer(container);
    sessionImpl.updateConnectorAndRepositoryName(); // TODO (CD) Can't we leave it to the session to call this?
  }

From a brief look I see that the session is already calling this method, but possibly in a different context? Have you tried to move the call?

More important, I wonder if the session package registry also has to be updated with the new session protocol as a PackageLoader...

Comment 3 Eike Stepper 2010-10-13 02:56:36 EDT

Created attachment 180725 [details]
Patch v2 - just reformatted

Comment 4 Caspar D. 2010-10-13 06:08:38 EDT

Committed to HEAD

Comment 5 Caspar D. 2010-10-13 06:36:54 EDT

> From a brief look I see that the session is already calling this method, but
> possibly in a different context? Have you tried to move the call?
> 

It is calling it during an actual failover. But the method needs to be 
called once to initialize the FailoverCDOSessionImpl. I'll see if it can be
moved to #activateSession.

> More important, I wonder if the session package registry also has to be
> updated with the new session protocol as a PackageLoader...

No I don't think so, because the packageLoader for the packageRegistry is 
in fact the session itself, which implements the PackageLoader interface
only to delegate to the "real" implementer, which is indeed the
sessionProtocol. (Note that this is inconsistent with the approach taken
for the commitInfoManager, the branchManager, and the revisionManager,
which do receive direct references to the sessionProtocol.)

But actually this is a good example of what I consider to be over-use of 
interface inheritance. It seems s'times you reason as follows: if A *has*
a B, then A might as well *be* a B, because it can delegate calls to the B instance it owns. I think this hurts conceptual clarity, but that's just
my humble opinion...

Comment 6 Caspar D. 2010-10-13 06:56:13 EDT

Created attachment 180751 [details]
Patch (incremental)

Ok this small additional patch moves the call (see earlier comment) 
to an override of #activateSession.

Comment 7 Caspar D. 2010-10-13 06:56:48 EDT

Reopening to have additional patch reviewed.

Comment 8 Eike Stepper 2010-10-13 10:40:39 EDT

What about the package registry? I doubt it would work properly after a fail-over when new package units get committed to the repo of the new session...

Comment 9 Eike Stepper 2010-10-13 10:44:47 EDT

(In reply to comment #8)
> What about the package registry? I doubt it would work properly after a
> fail-over when new package units get committed to the repo of the new
> session...

Sorry, I did not read your previous comment on this issue because your private email distracted me (processed first). Forget my repeated question ;-)

Comment 10 Caspar D. 2010-10-13 22:43:19 EDT

Committed to HEAD