[#926] Restart the session of a replication domain in one place, under the lock and the generation

[vharseko]

Fixes #926.

ExternalChangelogDomain.applyConfigurationChange() reached the session by a road of its own: domain.changeConfig(Set, Set) -> ReplicationDomain.restartService(), which was disableService() plus enableService() with neither serviceStateLock nor the session generation #892 gave the domain. #959 ([#943]) closed the first half of that: the ECL road runs under serviceStateLock now - changeConfig(Set, Set) and restartService() are overridden under it - and a domain which ownsItsSession(), shutting down or disabled for a total update, refuses the restart and reports it through onSessionRestartSuppressed() as adminActionRequired. What it left open is the counter: the restart it runs is still disableService() + enableService() with no sessionGeneration bump, so a replay thread which stopped the session and is waiting out its backoff cannot tell that restart from nothing, and the guard in restartSession() still reads isListenerShuttingDown() to stand in for it.

What changed

The lock and the generation live where the session does. serviceStateLock and sessionGeneration move from LDAPReplicationDomain to ReplicationDomain, next to sessionLock. The counter is bumped by disableService() and enableService() themselves - both final and taken under the lock now - rather than by hand at each of the places which stop or start a session, so every restart in the hierarchy is counted and no caller can forget to. A start which throws leaves the generation where it was, so the thread which stopped that session still owns it and may bring it back.

The guard in restartSession() is the generation alone. isListenerShuttingDown() was there to stand in for the restarts the counter could not see - changeConfig(), readAssuredConfig() - and there are none of those left. The starts at domain startup - startPublishService() from the constructor, startListenService() from start() - stay uncounted: no replay thread exists yet to hold a claim on that session. The method itself stays: processUpdate() still reads it.

A pair is atomic wherever it is called from. restartService() and readAssuredConfig() take their stop, change and start under serviceStateLock, as readFractionalConfig() already does, rather than only where applyConfigurationChange() holds the lock around them. The ownsItsSession() guard, onSessionRestartSuppressed() and the adminActionRequired report are #959's and are kept as they are; the isSessionRestartable() this PR used to carry was the same predicate, and is gone.

Tests

SessionRestartTest drives the road the issue names - applyConfigurationChange() -> ExternalChangelogDomain -> changeConfig() -> restartService() - against a live replication server, for a domain disabled for a total update and for one which has shut down. It failed against the base this PR was opened on; on master since #959 that road is refused by ownsItsSession(), and the test is kept as the coverage of it which goes through the ECL child of the domain configuration and through shutdown().

LDAPReplicationDomainConfigChangeTest.serviceStateLockOf() follows the field to ReplicationDomain, or externalChangelogConfigurationChangesTheSessionUnderTheServiceStateLock would fail on NoSuchFieldException.

What this does not close

• The lock-and-generation mechanism is pinned by no test. Nothing in src/test has a replay thread ask for a restart, so a stale claim declining to restart and a current one restarting are exercised by nothing; a restartService() which did not move the counter would go unnoticed. Follow-up, together with the RS-side observation of a restart which does happen and a fractional twin of assuredConfigurationIsAppliedToADomainWhichOwnsItsSession - see the review.
• The external changelog entry bypasses the total-update guard. LDAPReplicationDomain.isConfigurationChangeAcceptable() refuses a configuration change while ieRunning(); ExternalChangelogDomain.isConfigurationChangeAcceptable() returns true unconditionally, so a dsconfig set-external-changelog-domain-prop --set ecl-include:... during an initialize-from-remote-server restarts the session which is carrying the initialization. I found no way to hold ieRunning() open deterministically without a test-only hook in production code, so it is left out rather than shipped untested.

Review round 1

Rebased onto f559b0907a, over #959 ([#943]), #970 ([#951]), #972 ([#967]), #973 ([#928]) and #975 ([#927]). #959 landed in the same region between the base this was opened on and the review, and takes three of the review's points with it; the description above is rewritten for what remains, and the javadoc of serviceStateLock carries the lock-ordering paragraph #972 put on it. The point-by-point answer is in the comments.

[vharseko]

Rebased onto 5d176c6915. The branch was conflicting with four changes which landed in LDAPReplicationDomain while it waited - #971 ([#924]), #944 ([#901]), #945 ([#908]) and #948 ([#916]) - and the resolution keeps every one of them whole. Nothing in the diff of this PR changed: it is the same 301/82 it was before.

enable() - #971. It clears disabled before the session is started and wraps the start in a try/finally which puts the flag back when the start throws. This PR takes sessionGeneration++ out of that method, because enableService() counts the start itself now, and only once it succeeded. The structure of #971 stays as it is, minus that one line:

      disabled = false;
      boolean started = false;
      try
      {
        enableService();
        started = true;
      }
      finally
      {
        if (!started)
        {
          disabled = true;
        }
      }

Leaving the line where the merge put it would not have compiled either: sessionGeneration is private to ReplicationDomain on this branch, and the bump would have been a second one on a counter which is read as the identity of a session.

The field block - #944, #945, #948. replayLock, REPLAY_DRAIN_TIMEOUT_IN_MS and replayDrainTimeoutInMs were added where this PR takes serviceStateLock and sessionGeneration out, and #944 moved replayGiveUpDelayInMs away from the same place. All of the new fields are kept; what leaves is the two which move to ReplicationDomain, with their javadoc and the net.jcip.annotations.GuardedBy import, whose only use in this file was on sessionGeneration.

disable() - #948. It reordered the method and added the wait for the replay threads. The order is kept as #948 left it, and again only the bump goes:

      disabled = true;
      disableService(); // This will cut the session and wake up the listener
      awaitReplayDrained();
      state.save();
      state.clearInMemory();

Green on the rebased branch:

Tests run: 2,  Failures: 0, Errors: 0, Skipped: 0 -- org.opends.server.replication.plugin.SessionRestartTest
Tests run: 12, Failures: 0, Errors: 0, Skipped: 0 -- org.opends.server.replication.service.ReplicationDomainTest

ReplicationDomainTest is in there because #945 changed it and FakeReplicationDomain in the same package this PR moves the lock and the counter into.

[maximthomas]

praise: The fix is real and the consolidation is the right shape.

I ran SessionRestartTest at the merge-base (5d176c69): both cases fail there, and the road is reached synchronously from both disable() and shutdown(), so the two states the class covers are the two that matter. Moving the stop/start pair into disableService()/enableService() under serviceStateLock, with the generation bumped by the pair itself, removes the last unlocked pairs from readAssuredConfig()/readFractionalConfig(); "the configuration is applied either way" is the right contract for a domain that owns its session; and the description says why isListenerShuttingDown() can go — every restart is now counted — which made the guard change checkable rather than something to take on faith.

---

issue (blocking): The isSessionRestartable() arms of readAssuredConfig() and readFractionalConfig() are pinned by no test.

opendj-server-legacy/src/main/java/org/opends/server/replication/service/ReplicationDomain.java:3949 (readAssuredConfig)
opendj-server-legacy/src/main/java/org/opends/server/replication/plugin/LDAPReplicationDomain.java:851 (readFractionalConfig)

SessionRestartTest.changeEclIncludes() swaps only the ECL child on the same DomainFakeCfg, so needReconnection(config) is false and neither arm carrying the new term is evaluated; the assured/fractional suites change config on a live domain only. Deleting && isSessionRestartable() at L851, or reverting L3949-3960 to the unconditional disable/assign/enable, keeps every suite the PR names green — and that is the dsconfig ... assured-type during import-ldif road the description opens with.

A third case in the same harness pins it:

@Test
public void assuredChangeOnDisabledDomainIsAppliedWithoutASession() throws Exception
{
  // RS + domain.start() + assertTrue(domain.isConnected()) as in the two existing cases
  domain.disable();

  final SortedSet<String> replServers = new TreeSet<>();
  replServers.add("localhost:" + rsPort);
  final DomainFakeCfg assuredCfg = new DomainFakeCfg(baseDN, DS_ID, replServers,
      AssuredType.SAFE_DATA, 1, GROUP_ID, 1000, new TreeSet<String>());   // needReconnection() == true
  assertEquals(domain.applyConfigurationChange(assuredCfg).getResultCode(), ResultCode.SUCCESS);

  assertTrue(domain.isAssured(), "the configuration is applied either way");
  assertFalse(domain.isConnected(), "a disabled domain must not get its session back from a config change");
}

note: pre-existing and not yours — assuredConfig = config sits inside the needReconnection() branch, so a change of assured-timeout alone is never applied until a reconnection. Out of scope here.

---

suggestion (non-blocking): Add a positive twin — a case where the restart does happen and is observed on the RS side.

opendj-server-legacy/src/test/java/org/opends/server/replication/plugin/SessionRestartTest.java:129

getEclIncludes() at L129 is set at ReplicationDomain.java:3444 before restartService() is called, so the mutant restartService() {} — or isSessionRestartable() { return false; } — keeps every suite green (GroupIdHandshakeTest's assertion is also satisfied by the MonitorMsg best-RS re-election). The restart carries one thing only, the new eclIncludes/groupId to the RS and on to the other DSs via TopologyMsg; that road is what would silently break. The RS-side observable is a new DataServerHandler after the reconnect, so look it up inside the retry:

changeEclIncludes(domain, domainCfg);                      // live domain, no disable()
new TestTimer.Builder().maxSleep(5, SECONDS).sleepTimes(100, MILLISECONDS).toTimer()
    .repeatUntilSuccess(new CallableVoid()
{
  @Override
  public void call() throws Exception
  {
    final DataServerHandler ds = replicationServer.getReplicationServerDomain(baseDN)
        .getConnectedDSs().get(DS_ID);
    assertNotNull(ds);
    assertThat(ds.toDSInfo().getEclIncludes()).contains("cn");
  }
});

---

issue (non-blocking): The lock-and-generation mechanism in the title is pinned by nothing — a follow-up issue is fine by me.

opendj-server-legacy/src/main/java/org/opends/server/replication/plugin/LDAPReplicationDomain.java:3286 (restartSession)

src/test has zero references to sessionRestartRequested, runRequestedSessionRestarts or restartSession(, and the new test never has a replay thread ask for a restart. Deleting both sessionGeneration++, or replacing getSessionGeneration() != stoppedSession by false, stays green. The guard now rests on the generation alone (the dropped isListenerShuttingDown() is reasoned in the description, no objection), so the first test that reaches it should be the one that shows a stale claim declining to restart and a current one restarting.

---

todo: The sessionGeneration javadoc states an exclusivity the code does not keep.

opendj-server-legacy/src/main/java/org/opends/server/replication/service/ReplicationDomain.java:388

It says the session is stopped or started by "only disableService() and enableService()", and the description says the only uncounted start is start(). Neither holds: the constructor's startPublishService() (LDAPReplicationDomain.java:758) and the public startListenService() (L3261) start pieces of the session without a bump. Both run before a replay thread can hold a claim, so nothing breaks — but the invariant as written is false. Suggested:

 * Bumped every time {@link #disableService()} stops the session or {@link #enableService()}
 * starts it, both under {@link #serviceStateLock}. The starts at domain startup -
 * startPublishService() from the constructor, startListenService() from start() - are not
 * counted: no replay thread exists yet to hold a claim on that session.

---

todo: The isSessionRestartable() javadoc promises a session that does not always come back.

opendj-server-legacy/src/main/java/org/opends/server/replication/plugin/LDAPReplicationDomain.java:3949

"is read as the domain starts its session back" — enable() returns before disabled = false when loadDataState() throws (L4122-4134), and the flag stays set for the life of the server (#966). Suggested last sentence:

 * The configuration such a change carries is applied all the same; the session is not
 * restarted, and comes back only when the domain is enabled again.

---

suggestion (non-blocking): Say something when the change is stored but the restart is skipped.

opendj-server-legacy/src/main/java/org/opends/server/replication/service/ReplicationDomain.java:3464 (restartService)

Today restartService(), readAssuredConfig() and readFractionalConfig() all take the not-restartable arm silently and applyConfigurationChange() answers SUCCESS; on a domain stuck disabled (#966) the operator has no line telling them the new assured/fractional/ECL settings are waiting for a restart. One NOTICE (a logger.info(LocalizableMessage) reaches logs/errors) at the three sites, or one in isSessionRestartable()'s false arm, would do. Not a regression — at the base the same change brought up a session with an empty ServerState, which was worse.

---

nitpick (non-blocking): assertTrue(domain.isConnected()) straight after start() with no retry.

opendj-server-legacy/src/test/java/org/opends/server/replication/plugin/SessionRestartTest.java:58 (also L93)

In practice deterministic — ReplicationBroker.start() connects synchronously and the RS is bound first — so take it or leave it; GenerationIdTest wraps the same assert in repeatUntilSuccess.

[vharseko]

Rebased onto f559b0907a. #959 ([#943]) merged twelve minutes before this review, in the same region as this PR, and it changes the answer to most of it: the ECL road runs under serviceStateLock there already (changeConfig(Set, Set) and restartService() overridden under it), a domain which ownsItsSession() refuses the restart, onSessionRestartSuppressed() reports it as adminActionRequired, and assuredConfig = config is stored unconditionally. All of that is kept, and isSessionRestartable() is gone - it was ownsItsSession() with the sign flipped. What #959 did not do is count: its restart is disableService() + enableService() with no bump, so restartSession() still needs isListenerShuttingDown(). That is what the PR is now - the lock and the counter in ReplicationDomain, final disableService()/enableService() which count, the guard on the generation alone - and the description is rewritten for it.

issue (blocking) - the isSessionRestartable() arms of readAssuredConfig()/readFractionalConfig(). Verified as you describe it against the old base: changeEclIncludes() leaves needReconnection() false, neither arm ran, the mutant stayed green. On the rebased branch the arms do not exist: both methods are back on needReconnection && allowReconnection, with allowReconnection = !ownsItsSession() computed once by applyConfigurationChange() under the lock, as #959 left it, and this PR no longer touches that line in readFractionalConfig(). The assured half of the test you propose is on master as LDAPReplicationDomainConfigChangeTest.assuredConfigurationIsAppliedToADomainWhichOwnsItsSession - disable(), SAFE_READ, no listener thread, mode and timeout applied, adminActionRequired. The fractional half is pinned by nothing on master either; it belongs next to that test rather than in SessionRestartTest, and goes into the follow-up below.

note - assuredConfig = config inside the needReconnection() branch. #959, with assuredTimeoutIsAppliedAlthoughItNeedsNoReconnection pinning it.

suggestion - a positive twin observed on the RS side. Right that nothing observes the restart itself: eclIncludes is set before restartService() runs, and GroupIdHandshakeTest would be satisfied by the best-RS re-election off the next MonitorMsg. The DataServerHandler lookup inside the retry is the shape; follow-up with the next one.

issue (non-blocking) - the lock and the generation are pinned by nothing. Still true, and after the rebase it is all the PR does, so I am not hiding behind "follow-up is fine": one issue for the three tests - a FakeReplicationDomain in ReplicationDomainTest reading getSessionGeneration() around restartService() so that a restart which does not move the counter fails, the RS-side twin above, and the fractional twin - so that this round stays the move of the mechanism and the tests come as a change of their own.

todo - sessionGeneration javadoc. Rewritten as you suggest: bumped by the two service methods under the lock, and startPublishService() from the constructor and startListenService() from start() named as the uncounted starts. The description had the same error - it named start() only - and is corrected.

todo - isSessionRestartable() javadoc. The method is gone with the rebase; what says it now is #959's override of restartService() in LDAPReplicationDomain, which already names a domain left disabled by a failed enable() - "one which stays disabled ... never does".

suggestion - say something when the restart is skipped. #959: NOTE_REPLICATION_DOMAIN_SESSION_NOT_RESTARTED with adminActionRequired on the ConfigChangeResult, so it reaches the dsconfig operator rather than only logs/errors.

nitpick - assertTrue(domain.isConnected()) with no retry. Left as it is: ReplicationBroker.start() calls connectAsDataServer() synchronously under startStopLock, and the RS is bound first.

Two more things the rebase had to do: LDAPReplicationDomainConfigChangeTest.serviceStateLockOf() follows the field to ReplicationDomain, or externalChangelogConfigurationChangesTheSessionUnderTheServiceStateLock fails on NoSuchFieldException; and the javadoc of serviceStateLock carries the lock-ordering paragraph #972 put on it. The local run of the classes around this is queued behind three other builds on the same machine, so CI is the gate for this round.