[#986] Give back the changes a replay thread the pool stopped had parked

[vharseko]

A change which waits for another one is parked - RemotePendingChanges.checkDependencies() puts it in dependentChanges - and stays owned by the replay thread which parked it while that thread goes back to the pool and takes the changes which follow. getNextUpdate() is what hands it out again, to whichever replay thread clears the change it was waiting for, and that thread takes it over: the ownership of a parked change is deliberately held by a thread which is not replaying it right now.

That holds for as long as the parking thread comes back to the pool. It does not hold when the pool is stopped. MultimasterReplication.applyConfigurationChange() calls stopReplayThreads(), which sets the shutdown flag on every ReplayThread and joins them, and then createReplayThreads() makes a new pool. A thread which parked a change and came back to the queue is idle when it is stopped, and it ended still recorded as the owner of that change - a thread which does not exist anymore - while putRemoteUpdate() refuses every redelivery of a change a replay thread owns. On a domain which then goes quiet, that change is where this replica's ServerState, and every change behind it from every master, stops.

The trigger is wider than the number of replay threads: applyConfigurationChange() is the change listener of the whole cn=Multimaster Synchronization entry and stops the pool before it looks at what changed, so a modification of ds-cfg-connection-timeout does it too.

Where the give-back of #954 does not reach

It lives in the catch of replay(), and neither road out of a stopped pool throws:

• the idle thread is not inside replay() at all - it parked a change, met a getNextUpdate() which returned nothing, and returned to the queue;
• the thread which is inside one meets replayThreadShutdown at the top of the replay loop and leaves by abandonReplay(), which returns from replay() normally: what it was replaying is handed back, what it parked is not.

The thread gives back what it parked

ReplayThread.run() gives back, in a finally, the changes this thread parked - in every domain of this server, through LDAPReplicationDomain.giveBackChangesParkedByStoppingThread(). Done by the thread which owns them rather than by whoever stopped it, so the rule every road which reads ownership follows holds here as well: a change is given back by the thread it was handed to and by nobody else (#922). Doing it there is also what sees them all - the pool is shared by every domain of this server, and a thread may have parked a change in any of them, while a replay() knows only the domain it was replaying for. It covers the abandoned-replay road at the same time, so that road needs no give-back of its own.

The loop of run() moves into replayUntilStopped() so that the finally does not re-indent it.

The restart is for those changes and no others

A change which nobody owns is one only a new delivery brings back, so the session has to be restarted for it. It is run by the thread which stopped the pool, from applyConfigurationChange(), once the new pool is up - a change delivered again while the pool is empty would wait in the replay queue rather than be replayed - and last in that method, so the sessions which start run on the configuration this change carries. A thread on its way out is not held for a session either.

Only the restart these changes asked for is run there: restartSessionForChangesGivenBackByStoppedThreads() acts on a flag of its own, set when a stopping thread actually gave something back. A restart which a failed replay requested is left to the replay thread which asked for it - that one sits through the backoff this domain has reached on the change it can not apply, and running it from the configuration change would spend that wait on a road which never failed (#889).

The changes handed back stay listed and uncommitted, and stay among the changes the newer ones are checked against, the way a change whose replay failed does. They are handed back without a failure counted against them - they were never applied here - as on the road #954 takes. Message 318 is generalised: it is now the report of a parked change whose replay thread went away, whichever way it went.

Tests

UpdateOperationTest.aChangeParkedByAThreadThePoolStoppedIsDeliveredAgain: one replay thread, a change whose replay fails is the barrier, the change which follows it on the same entry is parked behind it by that thread, and ds-cfg-num-update-replay-threads is then changed - the pool is stopped and created again, the way an administrator has it. It watches dependent-changes-size go back to zero, then has the changes delivered again - nothing else sends them, they never travelled a session - and checks that the change which was parked is applied. It was watched failing on this branch without the fix: dependent-changes-size expected [0] but found [1].

Run locally on this head: UpdateOperationTest 33/33. That class is where #941 put aChangeAStoppedReplayThreadHeldIsGivenBackAndDeliveredAgain, the test of the change a stopped replay thread was holding, and it walks the applyConfigurationChange() this change adds a session restart to: it is the one test master has on the road this change touches, and it is green here.

The classes around it in one run: RemotePendingChangesTest 24/24, AssuredReplicationPluginTest 14/14, IsServerFailureTest 23/23, ModifyConflictTest 36/36, NamingConflictTest 8/8, PendingChangesTest 5/5, DependencyTest 3/3 - 146 with UpdateOperationTest, green together, and StateMachineTest 5/5 in a run of its own. A run of that set was what caught an earlier version of this change, where the configuration change ran every session restart which was pending rather than only the ones these changes asked for: it took the backoff of #889 away from the road which had asked for it.

Rebased on the heads of the stack, and on master

The branch is rebuilt on the current heads of what it is stacked on - #958, five commits now, the fifth being [#922] Pin the ownership index on the hand-out and the give-back, and assert the alert an OutOfMemoryError leaves behind, rebased onto master at 13d57e0, and the [#954] commit of #985 as it stands there, where the owner of the change is read before the parked changes are given back - and the [#986] commit applies on top of them with no conflict at all.

Moving the [#954] commit onto the current #958 met one conflict, in prose: the javadoc of changeBeingReplayed in RemotePendingChanges, where the fifth commit of #958 added the paragraph on who writes an entry of that index and #954 rewrote the sentence on the parked changes. It carries both.

The conflict which mattered is one git reported nowhere. #973 landed on master while this waited: a modify whose entry DN does not parse is reported once and stepped over rather than thrown on, so its CSN goes into the ServerState on the delivery which met it. ModifyMsgWithAnUnparseableOperationDN is that message, and the two tests of this stack which need a change whose replay fails and stays listed as the barrier the parked change waits behind - aChangeParkedByAnUnwoundReplayIsDeliveredAgain and aChangeParkedByAThreadThePoolStoppedIsDeliveredAgain - were built on it. Both sides merge without a marker and both tests lose their premise: the barrier is committed, nothing is parked behind it, and dependent-changes-size never reaches 1. They use ModifyMsgWhoseOperationRefusesAControl now, which #973 introduced for exactly the case those tests want - an operation which is built and fails before the CSN of the change is read, so the change is kept out of the ServerState and asked for again - and which is what the rebase of #958 moved its own failing messages onto. Four lines, the barrier delivery and the redelivery of it in each test.

The earlier round of this rebase, on #944, is still in place: the two tests shorten the replay-give-up-delay budget through setReplayGiveUpDelay(TEST_GIVE_UP_DELAY) with resetReplayGiveUpDelay() in the finally, in the middle of the test rather than at its head, so that the phase which parks a change behind one whose replay keeps failing runs on the default budget.

The class carries two ways of changing the number of replay threads: setNumberOfReplayThreads(), which #941 added and which applies a mocked configuration so that it can be run from a thread of its own, and the setNumUpdateReplayThreads() of this branch, which modifies ds-cfg-num-update-replay-threads on the configuration entry the way an administrator does. They are left as they are: what each test needs of that road is not the same.

Stacked on #985

This is written on top of #985, which is where the give-back of a parked change lives, and on #958 under it, which gives ownership an owner. The first six commits here are those PRs'; only [#986] is new. Once they are merged this PR is that one commit.

Fixes #986

Ordinal

NOTE_REPLAY_PARKED_CHANGE_GIVEN_BACK is 318, which is where #985 - the [#954] work this is stacked on - has it. The commit which moved it from 316 is gone with the rebase: the message comes in on that PR's commit at 318 now, and this branch only generalises its text. It is not merely untidy to have the two apart: the generated constant is the key name without its ordinal, so the same message at 316 here and at 318 there declares the field twice in ReplicationMessages once both are in, and the build stops - and git reports no conflict on it, the two lines sitting in different places in the file.

#935, #945 and #959 have landed, so 310-314, 319-320 and 326-327 are master's. The open PRs which add to replication.properties hold the rest with nothing claimed twice: 315-317 #958, #985, #988 (and #981, which carries the same three [#922] messages) · 318 #985, #988 · 321 #964 · 322 #968 · 323-324 #977 · 325 #981.

[vharseko]

Rebased on master at 2a7bb9d and force-pushed - the branch was conflicting, it is mergeable again. Nothing of the change itself moved: the four commits are the ones which were here, and the diff against master is what it was, bar the two lines below.

The one conflict was the import block of UpdateOperationTest, where #941 added TimeoutException and AtomicReference to the class this branch adds Supplier to. All three are there now. What #946 and #960 wrote in LDAPReplicationDomain and PendingChanges is elsewhere in those files and merged on its own, and the ordinals 315 and 316 this branch takes are still claimed by nobody in replication.properties.

The one thing worth a look is that #941 landed a test of the neighbouring road in the same class: aChangeAStoppedReplayThreadHeldIsGivenBackAndDeliveredAgain covers the change a stopped replay thread was holding, and it runs through the applyConfigurationChange() this change adds a session restart to the end of. It is green on this head - the thread it stops parked nothing, so nothing asks for the restart. UpdateOperationTest 23/23, and RemotePendingChangesTest, AssuredReplicationPluginTest, IsServerFailureTest, ModifyConflictTest, NamingConflictTest, PendingChangesTest, StateMachineTest, DependencyTest in one run, 112 in all, green together.

The merge leaves the class with two ways of changing the number of replay threads - setNumberOfReplayThreads() from #941, which applies a mocked configuration so that it can be run from a thread of its own, and setNumUpdateReplayThreads() here, which modifies ds-cfg-num-update-replay-threads on the configuration entry the way an administrator does. I left both: what each test needs of that road is not the same. Say the word if you would rather have one.

The PR description is updated to match. No review comments to answer yet - this is the rebase only.

[vharseko]

Ordinal moved: NOTE_REPLAY_PARKED_CHANGE_GIVEN_BACK 316 → 318 (4d1a83a).

Nothing catches this on the way in. The additions land in different parts of the file, so git merges
every pair of them without reporting a conflict - I merged all ten onto master to check, and the
result carried 310, 311, 315 and 316 twice each. The generator does not check either: it loads the
file into a Properties and keys on name and ordinal (MessagePropertyKey.compareTo), so both
sides compile. What comes out is two unrelated messages carrying one support ID, found by whoever
reads a log rather than by CI.

The open PRs which add to the file now hold 310-325 with nothing claimed twice:

310-313 #935 · 314 #959 · 315-317 #958, #985 · 318 #985, #988 · 319-320 #945 · 321 #964 ·
322 #968 · 323-324 #977 · 325 #981

No Java moved with it: the generated constant is the key name without its ordinal, so the rename is
confined to replication.properties. #935, #958 and #985 keep what they had.

This branch is not one of the six, and its collision is the one which does not merely muddy a log.
#985 - the [#954] work under this - already carries this message at 318. The generated constant is
the key name without its ordinal, so the same name at 316 here and 318 there declares
NOTE_REPLAY_PARKED_CHANGE_GIVEN_BACK twice in ReplicationMessages once both are in, and the
build stops. Git reports nothing on it: the two lines sit in different places in the file.

ERR_ERROR_REPLAYING_CHANGE_315 is the [#922] message and stays put. The description said the
ordinals this branch takes were claimed by nobody and that message 316 is generalised; both lines
are corrected there.

[vharseko]

Rebased, and the branch is mergeable again (8c8323f). Nothing of the change itself moved.

master took 319 and 320 with #945 while this waited, at the end of replication.properties where
this branch adds its own - that is the conflict GitHub was reporting. It is merged rather than
renumbered: 315 to 318 are where the stack has them, 319 and 320 are master's.

The conflict which mattered was reported nowhere

#944 replaced LDAPReplicationDomain.getReplayGiveUpDelay() and setReplayGiveUpDelay(long) with
the configured replay-give-up-delay property, and the two tests of this stack which shorten that
budget - aChangeParkedByAnUnwoundReplayIsDeliveredAgain and
aChangeParkedByAThreadThePoolStoppedIsDeliveredAgain - were still calling the setters. Both sides
merge without a marker and the class stops compiling; a rebase which only answered what git asked
would have pushed that.

They take the road the rest of UpdateOperationTest takes now - setReplayGiveUpDelay(TEST_GIVE_UP_DELAY),
resetReplayGiveUpDelay() in the finally - which is what the two cases of #958 already do. The
budget is still shortened where the setter was called, in the middle of the test rather than at its
head: the phase before it parks a change behind one whose replay keeps failing, and it runs on the
default budget the way it did. The reset is a delete which modifyDomainConfig() treats as done
when the attribute is not there, so a test which fails before the shortening still cleans up without
replacing its own failure.

Rebuilt on the heads of the stack, not on the copies it carried

This branch was carrying the revisions of #958 and #985 as they were when it was written. It is
rebuilt on their current heads instead - #958 at 31e633d, with the rounds of review it has had
since, and the [#954] commit of #985 over it - and then rebased onto master at 21d03d5, where
it applies with no conflict at all. Six commits: four of #958, one of #985, and the one which is new
here.

Moving the [#954] commit onto the current #958 met four conflicts, all of them prose: the javadoc
of getChangeOwnedByCurrentThread() and of
theChangesParkedAsDependenciesAreNotOwnedByTheThreadWhichParkedThem(), the comment in the catch
of replay(), and the block in replication.properties. Each of them now says what both rounds
say - the lookup which allocates nothing, and the parked changes which are given back on a road of
their own.

The [#986] Take an ordinal in replication.properties nothing else claims commit is gone with the
rebase: 318 comes in on #985's commit, and this branch only generalises the text of that message.

Run on this head

UpdateOperationTest 31/31, UpdateOperationTest 31/31, and the classes around it in one run - RemotePendingChangesTest 24/24, AssuredReplicationPluginTest 14/14, IsServerFailureTest 23/23, ModifyConflictTest 36/36, NamingConflictTest 8/8, PendingChangesTest 3/3, DependencyTest 3/3 - 142 in all, green together. StateMachineTest 5/5 on its own: in that run its setUp met the administration connector of another test's server on 0.0.0.0:65534.

The description is updated to match.

[vharseko]

Rebased, and the branch is mergeable again (dd3cafe). Nothing of the change itself moved.

Rebuilt on the heads of the stack

The branch is rebuilt on the current head of #958 - five commits now, the fifth being
[#922] Pin the ownership index on the hand-out and the give-back, and assert the alert an OutOfMemoryError leaves behind - rebased onto master at 13d57e0, and on the [#954]
commit of #985 as it stands there (13be038), which is where the owner of the change is read
before the parked changes are given back. The copy of that commit this branch was carrying
was behind it. Seven commits: five of #958, one of #985, and the one which is new here.

Moving the [#954] commit onto the current #958 met one conflict, in prose: the javadoc of
changeBeingReplayed in RemotePendingChanges, where #958's fifth commit added the paragraph
on who writes an entry of that index and #954 rewrote the sentence on the parked changes.
It carries both now. The [#986] commit applied with no conflict at all.

The conflict which mattered was reported nowhere

#973 landed on master in the meantime: a modify whose entry DN does not parse is now
reported once and stepped over rather than thrown on, so its CSN goes into the ServerState
on the delivery which met it. ModifyMsgWithAnUnparseableOperationDN is that message, and
the two tests of this stack which need a change whose replay fails and stays listed as the
barrier the parked change waits behind - aChangeParkedByAnUnwoundReplayIsDeliveredAgain
and aChangeParkedByAThreadThePoolStoppedIsDeliveredAgain - were built on it. Both sides
merge without a marker and both tests lose their premise: the barrier is committed, nothing
is parked behind it, and dependent-changes-size never reaches 1.

They use ModifyMsgWhoseOperationRefusesAControl now, which #973 introduced for exactly
the case those tests want - an operation which is built and fails before the CSN of the
change is read, so the change is kept out of the ServerState and asked for again - and which
is what the rebase of #958 moved its own failing messages onto. Four lines, the barrier
delivery and the redelivery of it in each test; the rest of both tests is what it was.

Run on this head

UpdateOperationTest 33/33 - both tests of this stack among them - and the classes around it in one run: RemotePendingChangesTest 24/24, AssuredReplicationPluginTest 14/14, IsServerFailureTest 23/23, ModifyConflictTest 36/36, NamingConflictTest 8/8, PendingChangesTest 5/5, DependencyTest 3/3 - 146 in all, green together. StateMachineTest 5/5 in a run of its own.

The description is updated to match.