[#954] Give back the changes a replay which is unwound parked as dependencies - #985
Conversation
vharseko
added
bug
replication
tests
1b3af39 to
240fadc
Compare
|
Rebased onto the head of #958. That branch grew a third commit while this sat - The ordinal is 318#958 claims 315, 316 and 317 now, which is what its own comment announced. #981 is on the same branch and also took 316, so it lands on 319 rather than on this. The end-to-end test unwinds the replay another wayIt drove It now uses The last resort runs the restart for these too#958's fallback runs Two javadocs which the third commit made untrue
Runs
The description is updated to match. |
240fadc to
13be038
Compare
|
Rebased onto the head of #958 - the branch conflicted with master and with that PR, which has grown a fourth commit since this was last pushed, The owner is read before the parked changes are given backThat fourth commit turns "which change does this thread own" into a plain map read which takes no lock and allocates nothing, because the give-back on the way out of an unwound replay runs on the road out of a JVM which has just refused an allocation. This PR was written when that lookup still walked the pending changes, and it put its own give-back first. Left as it merged, the give-back of the parked changes - which allocates: the list of what it released, and the line which reports each one - ran before anything had been read, so a throw from it reached the last resort of The read is first now, and the give-back follows it. The order of the roads is unchanged - the parked changes are still handed back before the road of the change this thread was replaying restarts the session, which is the whole point of where the call sits. Two comments the fourth commit made untrue
The end-to-end test takes the configured budget
The ordinals319 and 320 went into master with #945 while this sat, which is where Runs
The description is updated to match. |
13be038 to
48420fa
Compare
|
Rebased onto the head of #958, which is rebased onto master. That branch grew a fifth commit, The javadoc of the ownership indexIt conflicted where both branches rewrote it. This PR's sentence stays - a parked change is given back by One comment the fifth commit made untrueThe OutOfMemoryError arm of the ack said the give-back on the way out of The barrier of the end-to-end testIt was Ordinals318 is still nobody's: master holds 319, 320, 326 and 327. Runs
The description is updated to match. |
…wound parked as dependencies A change which waits for another one is parked and stays owned by the replay thread which parked it: getNextUpdate() is what hands it out again, to whichever thread clears the change it was waiting for. A replay which is unwound leaves that thread without the road back - it takes the next delivery off the shared queue - so the change stayed owned by a thread which never came back to it, while every redelivery of a change a replay thread owns is refused as a duplicate. On a domain which then went quiet that change was where this replica's ServerState, and every change behind it from every master, stopped. RemotePendingChanges gives back the changes the calling thread parked and unparks them in the same step, under both locks, so that only one road can hand a change out: a change released while it is still listed as waiting would be handed to the thread getNextUpdate() gives it to and to the thread which takes over the delivery which follows. The changes another thread parked are left alone, as everywhere else. replay() gives them back before the road of the change it was replaying runs, since that road restarts the session and a change which is still owned when the replication server sends it again is turned down. They are handed back without a failure counted against them - they were never applied here - and the session is restarted for them. Which change this thread owns is read before they are given back, and not after. The read is a plain map lookup which allocates nothing, and the give-back below it allocates - it builds the list of what it released and the line which reports each one. A throw from it on the road it exists for, a JVM which has just refused an allocation, would otherwise reach the last resort of replay() with nothing read, and the change this thread was replaying would be left listed, uncommitted and owned by a thread which is ending: the wedge OpenIdentityPlatform#922 is about, one road over. The order of the roads is unchanged - the parked changes are still handed back before the road of the change this thread was replaying restarts the session. Rebased onto the head of OpenIdentityPlatform#958, itself rebased onto master. That branch grew a fifth commit, "[OpenIdentityPlatform#922] Pin the ownership index on the hand-out and the give-back, and assert the alert an OutOfMemoryError leaves behind": the javadoc of the ownership index conflicted where both branches rewrote it and is merged, and the comment that commit wrote on the OutOfMemoryError arm of the ack - the give-back finds nothing once the change was committed - now says which parked changes the give-back does hand back on that road. Master's OpenIdentityPlatform#928 steps over a modify whose entry DN does not parse rather than asking for it again, so the end-to-end test builds the change it parks behind from an operation which is built and then refused, the way the tests of OpenIdentityPlatform#958 do since that merge; it takes the configured replay give-up budget, since OpenIdentityPlatform#901 replaced the setters it used.
48420fa to
06ace4f
Compare
|
Rebased onto master now that #958 is merged. Nothing had to be decided this time: the head of #958 this sat on is the tree master took in 776339a, so the one Ordinals: 315, 316 and 317 are in master with #958; 318 is still only this PR's. The description says "built on #958" instead of "stacked on #958" and names the master commit it is in. |
1 participant
A change which waits for another one is parked -
RemotePendingChanges.checkDependencies()puts it independentChanges- and stays owned by the replay thread which parked it while that thread goes on to 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 and keeps taking changes. It does not hold when its
replay()is unwound while it still holds one. The change is then owned by a thread which will never come back to it, and nothing else asks for it:putRemoteUpdate()refuses every redelivery of a change a replay thread owns, and amarkInProgress()which returnsfalsehas the replay thread carry on without reachinggetNextUpdate(). It is handed out again only when some other change is replayed on this domain - so 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 give-back unparks
RemotePendingChanges.releaseParkedChangesOwnedByCurrentThread()drops the owner of every change the calling thread parked and takes it out ofdependentChangesin the same step, under both locks. One step for both, because a change which was released while it is still listed as waiting would be handed to the threadgetNextUpdate()gives it to and to the thread which takes over the delivery which follows - the double replay the ownership is there to prevent (OPENDJ-1115). That hazard is why the give-back written for #922 left parked changes alone, and unparking them in the same step is what closes it.The changes another thread parked are left alone, as on every other road which reads ownership (#922): that thread may still be inside the dependency checks which parked the change - they park it once per dependency it has - so a change released under it would be listed as waiting again a moment later, and handed out while the delivery which took it over is being replayed.
What is given back stays listed and uncommitted, and stays among the changes the newer ones are checked against, the way a change whose replay failed does: it is not in the data, so it holds the ServerState back and the changes which follow it keep waiting for it.
Where it runs
replay()gives them back at the top of itscatch, before the road of the change this thread was replaying: that road restarts the session, and a change which is still owned when the replication server sends it again over the new session is turned down as the duplicate it looks like - the one delivery which could have taken it over. What is still requested is run afterwards, because that road may have had no restart to run at all: this thread owned no change, or the change it owned was given up on.Which change this thread owns is read before they are given back, and not after. That read is a plain map lookup which allocates nothing - #958 made it one - and the give-back below it allocates: it builds the list of what it released and the line which reports each one. A throw from it on the road it exists for, a JVM which has just refused an allocation, would otherwise reach the last resort of
replay()with nothing read, and the change this thread was replaying would be left listed, uncommitted and owned by a thread which is ending - the wedge #922 is about, one road over. The order of the roads is unchanged: the parked changes are still handed back before the road of the change this thread was replaying restarts the session.They are handed back without a failure counted against them - they were never applied here, so the give-up budget which decides when this replica skips a change it can not apply is not this delivery's to spend, the way it is not for a change abandoned by a replay thread which is stopping. The delivery which carried one is counted as processed instead: its ack was never published, since the ack of a parked change is published by the delivery which replays it, and that count is of the deliveries this replica took off the session. The receive window it holds is not given back and does not need to be - the session it came over is about to be restarted, and a session which starts is given its window anew.
The last-resort
catchofreplay()runsrunRequestedSessionRestarts()for these too. #958 has that fallback run the restart itself rather than leave a request the two readers of which both need a later failed replay in this domain, but it ran it under the change this thread was replaying: a give-back which released the parked changes and then threw formatting their report - the road anOutOfMemoryErrorleads to - would have left them unowned with nobody to ask for them, on a thread which owned no change of its own. The restart is asked for before they are reported, and it is now run whichever of the two roads asked.The message this adds is 318: 310-313 are claimed by #935 and 314 by #945; 315, 316 and 317 are in master with #958, 319 and 320 with #945, and 326 and 327 with #959. 318 sits between them and is still nobody else's. #981 has moved on to 325.
Which throws reach it
Two roads unwind
replay()now, and neither is the one this PR was first written on.An
Errormet replaying a change does not reach it: #922 reports it and takes the ordinary road of a failed replay, which gives the change back itself. A throw from publishing the ack does not either, since #958: publishing an ack says nothing about whether the change was applied, so it is reported underERR_ACK_NOT_PUBLISHEDand the replay carries on to the road the change itself decided.What is left is an
OutOfMemoryError, and a throw from what the replay runs once the ack of the delivery is out - the give-back of a change which failed, and thegetNextUpdate()which is the one drain of the changes parked behind a change which was committed. That second road is past everycatchthe replay itself has, and it is the one the end-to-end test drives.Tests
RemotePendingChangesTest: a change parked by a replay which is unwound is given back and taken over by the next delivery; a change which was given back is not handed out as a dependency anymore; the changes another thread parked are left alone.UpdateOperationTest: end to end, with one replay thread so that the change which is parked and the replay which is unwound after it are the same thread's. A change whose replay fails is the barrier - one whose operation is built and then refused (ModifyMsgWhoseOperationRefusesAControl), so that it is asked for again rather than stepped over the way Replication: the permissive-modify check dereferences a null entry DN when the DN does not parse #928 steps over a modify whose entry DN does not parse - the change which follows it on the same entry is parked behind it, and a delivery whose replay is unwound once its ack is out (ModifyMsgWhoseReplayIsUnwoundAfterItsAck, from [#922] Give a change back when the replay which owns it is unwound #958) unwinds the replay which holds it. It watchesdependent-changes-sizego 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. The barrier is held by a budget which is not spent while the test sets up and is shortened once the parked change is back: it takes the configuredreplay-give-up-delayroute, since Make the replay retry budget of a replication domain configurable instead of a constant with a test-only setter #901 replaced the setters it used.Every one of them was watched failing: the unit tests against a give-back which returns nothing and against one which does not check the owner, the end-to-end one against
replay()without the call.Run locally on this head:
UpdateOperationTest32/32,RemotePendingChangesTest24/24,AssuredReplicationPluginTest14/14,ModifyConflictTest36/36,NamingConflictTest8/8,StateMachineTest5/5,DependencyTest3/3 - 122 tests,Skipped: 0.Built on #958
This is written on top of #958, which is where the give-back on the way out of an unwound replay lives and which gives ownership an owner - without it there is no "the changes this thread parked" to ask for. #958 is in master since 776339a, and this PR is the one commit
[#954]on top of it.Left out
A replay thread which ends while it holds a parked change without anything being thrown: the road a replay abandoned by a stopping thread takes, and - wider -
MultimasterReplication.applyConfigurationChange(), which stops the whole pool whends-cfg-num-update-replay-threadschanges and takes the parked changes of every idle thread with it. It is the same defect, but nocatchofreplay()is on the way and the changes are owned by threads which are gone rather than by the one asking, so it wants a sweep of its own rather than a line here. It is #986.Fixes #954