Conversation
vharseko
added
bug
replication
concurrency
58df624 to
8f9a7fd
Compare
|
Force-pushed: rebased on The conflict was one hunk, the import block of What the rebase actually brought is worth a look, because it settles something this PR had only asserted. The last section of the description said the collapsed restarts of a It survives for a reason worth stating rather than leaving as a passed test. That test stops a replay thread while it holds a change and then reads the assured ack the abandoned delivery published. Under this change Run after the rebase, all green:
The two The description has been updated: the SHA to review is now 8f9a7fd, and the closing section says what became of the conflict it predicted. One thing unchanged by the rebase: this is still stacked on #958, which is itself conflicting with |
|
Ordinal moved: Nothing catches this on the way in. The additions land in different parts of the file, so git merges The open PRs which add to the file now hold 310-325 with nothing claimed twice:
No Java moved with it: the generated constant is the key name without its ordinal, so the rename is This branch is not one of the six: 316 was free where it sits, on the earlier [#922] commits, from |
31d7dfa to
0e91a8f
Compare
|
Force-pushed: rebased onto the current #958 at 31e633d (was 00fead7, the #958 of two review rounds ago), which sits on This time the conflict was not one of hunks. #958 went through two rounds since this was opened, and both reached into the code this PR changes, so what git reported in
The mechanical three: #901 removed One consequence for the description's "say plainly" paragraph: the precedent it named for the test hook, Run after the rebase, all green:
The description is updated: the SHAs, the bullets the two #958 rounds touched, and a closing section which says what was merged and how. |
0e91a8f to
ca9ba89
Compare
|
Force-pushed: rebased onto the current #958 at 9618f12 (was 31e633d), which sits on The two commits of this PR are what they were: 506 insertions and 67 deletions over the same five files. The one difference from the previous version is twelve lines of a comment #958 round 4 grew inside the What put both PRs back into conflict is worth a line, because it was not textual. #928 (#973) removed the NPE in Run after the rebase, all green:
The description is updated: the SHAs and the base. |
ca9ba89 to
823980c
Compare
|
Force-pushed: rebased onto The conflict GitHub reported was the squash, not the code. 776339a is #958's 9618f12 squashed onto Ordinal 325 is still claimed by nothing on Run on the new head: The description is updated: the stacking paragraph is gone, the SHAs and the base, and the closing section says what the squash did to the branch. |
823980c to
3592ab5
Compare
|
Force-pushed: rebased onto The conflict was one hunk, the tail of Nothing in the Java met #964: it changes About the red leg on the previous head (JDK 26): it was Run on the new head: The description is updated: the SHAs, the base, and the closing section says what #964 put in the file. |
… has run The recovery from a failed replay takes the request before it runs the restart, and restartSession() stops the session in its first synchronized block and starts it again in the second. Anything thrown in between left the request cleared and the session stopped, and nothing asked for it again: no change is delivered over a session which is down, so no replay fails and no thread comes back to the recovery. The domain sat out of the topology - the change it could not replay still owned by the replication server, its ServerState stopped behind it - until the server was restarted or a configuration change happened to restart the service. The throw is narrow but not theoretical: enableService() is the one call on that road nothing guards, while the reconnect loop of the broker wraps the same connectAsDataServer() in a catch, so every failure that path is written to survive is fatal to this one. The request is a SessionRestartRequests now. It is still taken before the restart runs, as it must be - a change released while a restart is under way is not one that restart asks for - and it is given back when the restart could not run, with the backoff whatever it was asked for with, since a session which can not be started is what that wait exists for. The wait belongs to the request rather than to the thread which runs it: a request made by a replay thread on its way out is owed none, one made by a failing replay is owed one, and either thread can end up running the other's request. Nothing else runs a request which is left standing, so the state checkpointer of the domain does, once a second: it is the one thread a domain has for as long as it is up, whatever its session is doing. That also lets abandonReplay() ask for the restart rather than run it - the threads of the pool are stopped one after the other and joined, so a live num-update-replay-threads change cost one stop and reconnect per thread which was replaying a change, none of them waiting, while the configuration change waited for all of them - and it runs the bare hand-back of issue OpenIdentityPlatform#922, which asked for a restart nothing was going to run. The backoff is waited on a monitor rather than slept through, and a domain which is going away or is being disabled wakes it: the thread which holds it is a replay thread the shutdown of the pool joins, or the checkpointer the shutdown of the domain waits for.
…nothing else claims
3592ab5 to
2a2766f
Compare
|
Force-pushed: rebased onto The conflict was one hunk, the field block of Worth a line, since #974 reached into the same restart: it counts the generation inside Ordinal 325 is still claimed by nothing on Run on the new head, all green:
The description is updated: the SHAs, the base, and the closing section says what #974 did to the file. |
1 participant
Fixes #925.
Was stacked on #958, which has landed (776339a). The branch now sits on
masterdirectly - at cebef54, which is #926 (#974) - and the diff is the two commits alone: 0b35ffd ([#925] Keep asking for a session restart until it has run) and 2a2766f, which moves its message ordinal. The rounds #958 went through while this was open reached into the same code, so the rebases along the way were a merge of decisions rather than of hunks - the last section says which, and it still describes how this change meets what #958 put onmaster.What was wrong
runRequestedSessionRestarts()takes the request before it runs the restart, andrestartSession()stops the session in its first synchronized block and starts it again in the second. Anything thrown in between left the request cleared and the session stopped, and nothing asked for it again: no change is delivered over a session which is down, so no replay fails and no thread comes back to the recovery. The domain sat out of the topology - the change it could not replay still owned by the replication server, its ServerState stopped behind it - until the server was restarted or a configuration change happened to callrestartService().Worth being precise about the trigger, because the issue text names one which does not happen:
broker.start()on an unreachable replication server does not throw.connectAsDataServer()swallows every connection failure -performPhaseOneHandshake()catchesConnectException,SocketTimeoutExceptionandException,connectToReplicationServer()catchesException- and leaves the domain in degraded mode, where the listener thread reconnects on its own. What makes the throw reachable is narrower and, in one respect, damning:enableService()is the one call on that road nothing guards, while the reconnect loop of the broker wraps the sameconnectAsDataServer()in acatch (Exception). Every failure that path is written to survive is fatal to this one, and what is fatal about it is the missing listener thread: it is the reconnection engine, so a domain which loses it has nothing left to bring the session back.What changed
SessionRestartRequests(new) holds what the domain has been asked for -NONE<NOW<AFTER_BACKOFF, merged by taking the strongest. The request is still taken before the restart runs, which it must be: a change released while a restart is under way is not one that restart asks for, its delivery would be turned down as a duplicate of a change a replay thread still owns. Clearing the flag after the restart instead - the other half of the suggestion in the issue - would swallow exactly those requests.NOWrequest given back bare would have the retry hammer once a second. [#922] Give a change back when the replay which owns it is unwound #958 has since put the request back on its own (round 2, blocking 4) - afinallywhich sets the flag again - and this replaces it: the give-back carries the backoff, and, the point of this PR, something runs what was given back. Under [#922] Give a change back when the replay which owns it is unwound #958 alone the restored request is run by "the next failed or abandoned replay of this domain", and on a session which is down there is none.stopReplayThreads()sets the shutdown flag of each thread in a loop, so the window is the one between the first flag and the last. It costs nothing to close it here, since the request had to carry its own state anyway. The thread anOutOfMemoryErroris ending, which [#922] Give a change back when the replay which owns it is unwound #958 exempts from the backoff for the same reason, asks forNOWas well.shutdown()takes that monitor to wake it - and swallows and reports a failure rather than end on it: a domain whose checkpointer is gone saves no ServerState, andshutdown()waits for that thread.abandonReplay()asks for the restart rather than running it (the second asymmetry). The threads of the pool are stopped one after the other and joined, so a liveds-cfg-num-update-replay-threadschange cost one stop and reconnect per thread which was replaying a change - none of them waiting, each one a listener join plus a full handshake, and the configuration change waited for all of them. One restart is all the replication server needs to send back every change which was handed back.replay()keeps running its restart itself, as [#922] Give a change back when the replay which owns it is unwound #958 decided in its round 1 (non-blocking 4): on that road the thread is likely ending and the report is the line an operator acts on. What this branch adds there is the same as everywhere else - a restart which throws leaves its request standing, and the checkpointer runs it, where before the request went out with the thread.disable()/shutdown()wake it. This is not in the issue; it is what the checkpointer needs in order not to hold a domain going down for up to ten seconds, and it removes the same stall for a replay threadstopReplayThreads()is joining.Tests
SessionRestartRequestsTest(new)UpdateOperationTest.aSessionRestartWhichCouldNotRunIsRunAgain(new)UpdateOperationTestmasterplus this oneRemotePendingChangesTest/AssuredReplicationPluginTest/DependencyTestThe new test was checked against both mutations it exists for: with the checkpointer's call removed it fails, and with the give-back removed it fails - each half of the fix is load-bearing on its own.
Two things to say plainly rather than bury.
failNextSessionRestarts(int)/getSessionRestartFailuresLeft()are@VisibleForTestingand there is a branch inrestartSession()which production never takes. Nothing else reachesenableService(): connection failures are swallowed inside it, and the machinery #958 added throws from a replay, not from a restart. The precedent this PR first named,setReplayGiveUpDelay(), is gone - #901 replaced it with the configuredreplay-give-up-delay- so the one left in the class issetReplayDrainTimeout()from #945. If you would rather have no injection point in the class, the alternative I can see is dropping the integration test and keeping only the unit test of the holder, which leaves the give-back and the checkpointer unpinned.The collapsed restarts of a
num-update-replay-threadschange have no test of their own here. Pinning "one restart rather than one per thread" needs the replay parkingShortCircuitPlugingets in #941, and writing one here would have duplicated it.#941 has since landed (d0422c6), and
aChangeAStoppedReplayThreadHeldIsGivenBackAndDeliveredAgainsits in this class next to the change it was claimed to survive, and it is green. It is worth saying why it survives rather than only that it does. That test stops a replay thread while it holds a change and reads the assured ack the abandoned delivery published; under this changeabandonReplay()no longer restarts the session itself, so the ack now goes out over a session which is still up, where before it raced the teardown. The test is not merely still valid - it is the less brittle for this change. Its comment on reading the ack rather than its ordering ("an ack published after the hand-back reaches it all the same") describes a race this change removes.Rebased onto #958 as it went, and onto
masteronce it landedTwo rounds of #958 landed on the same code between the opening of this PR and now, and git reported six hunks in
LDAPReplicationDomain. Three were mechanical - #901 removed thereplayGiveUpDelayInMsfield and its setters this branch had sat next to, so they go; theoutOfMemoryparameter #958 gaverecoverFromReplayFailure()becomes the request beingNOW. Three were decisions:finallyaroundrestartSession()which sets the flag again. Replaced bygiveBack(AFTER_BACKOFF), for the two reasons above: the backoff travels with the request, and the checkpointer runs it. The concrete trigger that round named - the operating system refusing the listener thread - is kept in the comment.replay()run the restart rather than leave a request. Kept as decided there; the request is madeNOWfirst, so a restart which throws is given back and run by the checkpointer instead of going out with the thread. The earlier text of this PR, which said that road "asked for a restart nothing was going to run", described the [#922] Give a change back when the replay which owns it is unwound #958 of the time and is gone.ERR_ACK_NOT_PUBLISHED_316and the 316 this branch first took met in one hunk, and git did report it this time. 0e91a8f resolves it toERR_REPLAY_SESSION_RESTART_FAILED_325; the file has no ordinal twice. The open PRs which add toreplication.propertiesstill hold 310-325 with nothing claimed twice.The diff of the two commits against
masteris 506 insertions and 67 deletions over the same five files as before.Rebased once more onto #958 round 4 (9618f12, on
masterat 13d57e0). Round 4 only grew the comment inside thefinallythis branch replaces, so the resolution above stands and those lines go with the block; the twelve of them are the whole difference from the previous version of the two commits. What put both PRs back into conflict was #928 (#973), which removed the NPE #958's test scaffolding relied on to fail a replay before the CSN is read - #958's own rebase moved that scaffolding toModifyMsgWhoseOperationRefusesAControl, and this branch sits on it as it is.#958 has since landed as 776339a, a squash of the 9618f12 this branch sat on, with the same tree. Rebased onto
masterthere: the five commits it carried went, the two of this PR moved without a hunk to resolve, and the tree of the new head is the tree of the old one -git diff ca9ba89 823980cis empty.Rebased once more onto
masterat 9ff409b, where #964 landed. It tookWARN_CHANGELOG_READ_AGAIN_FOR_MISSING_CHANGES_321at the end ofreplication.properties, next to the message this branch adds, and that was the whole conflict: one hunk, both sides kept, 321 above 325. Nothing in the Java met #964 - it changes the replication server's catch-up, this branch changes the domain's restart - and the two commits are what they were, 506 insertions and 67 deletions over the same five files. 325 is claimed by nothing onmaster, which holds 310-317, 319-321 and 326-327; the file has no ordinal twice.Rebased once more onto
masterat cebef54, where #926 (#974) landed. It movedserviceStateLockandsessionGenerationup fromLDAPReplicationDomainintoReplicationDomain, and this branch addssessionRestartFailuresToInjectright where they used to be - one hunk, the field block, resolved by takingmaster's side and keeping only the new field. Everything else merged without a word, and arange-diffagainst the previous version of the two commits shows context only: the moved declarations, and thesessionGeneration++#974 took out ofrestartSession()next tofailSessionRestartIfATestAskedFor(). #974 reached into the same restart, so the meeting point is worth a line: it counts the generation insideenableService(), and only once the session is up, so a start which threw leaves the generation where the stop put it. The injected failure of this branch sits between the generation guard andenableService(), which is the same case - the thread which stopped the session still owns it, and the retry the state checkpointer runs goes through a freshdisableService()as any other restart does. Nothing in this branch had to move for it. The two commits are still 506 insertions and 67 deletions over the same five files; 325 is still claimed by nothing onmaster, and the file has no ordinal twice.