[#950] Announce a ReplicaOfflineMsg before it is published, not after it may have been forwarded - #978
Conversation
8175d6e to
848c47c
Compare
|
Rebased onto master now that #946 has landed. The conflict both PRs predicted is resolved and the description above is updated to match; no review had been posted yet, so nothing here answers a review comment. The two changes met in the same three files.
final CSN offlineCSN = pendingChanges.putReplicaOfflineMsg();
if (offlineCSN == null && logger.isTraceEnabled())
{
/*
* The announcement itself is made where the message is published, so nothing has to be
* reported here: a message which never reached the wire was never announced either.
*/
logger.trace("Replica " + getServerId() + " of domain baseDN=" + getBaseDN()
+ " could not announce itself offline: a change which is still in flight holds"
+ " the message back, and " + pendingChanges.size() + " change(s) are pending");
}
One case needed more than a merge, and it is worth naming.
|
|
For the record, since the run on the pre-rebase head 8175d6e went red: The map this branch moves the write of - It looks like #924; the evidence, and a second sighting of the same signature on another |
848c47c to
ec70866
Compare
|
Rebased onto master ( Only This finishes the note above about the red run on the pre-rebase head. That failure now has a name: Verified on the rebased branch rather than on the old head:
|
ec70866 to
4aeb3b3
Compare
|
Rebased onto master ( The conflict was with #976, in the final CSN offlineCSN = msg.getCSN();
replicaOfflineAnnouncer.announce(offlineCSN);
if (domain.publish(msg))
{
publishedOfflineCSN = offlineCSN;
}
else
{
// The broker wrote it to no session, so nobody will forward what was announced.
replicaOfflineAnnouncer.withdraw(offlineCSN);
}
#947 changed Tests, on the rebased head:
Both regressions were watched: with the announcement moved back behind The description above is updated to match. |
maximthomas
left a comment
There was a problem hiding this comment.
praise: The order is now the right one, and a refusal is no longer silent.
- Announcing before
domain.publish()closes the #950 window by construction: a forward reported from inside the publish can no longer run ahead of the announcement it clears. PendingChanges.ReplicaOfflineAnnouncerkeepsPendingChangesoffDSRSShutdownSync; the seam is two methods, and the tests build their own announcer through it.replicaOfflineMsgNotSentwithdraws withremove(key, value)under acsn.equalsguard, so a stale withdrawal cannot take a fresher entry with it.refuseWhilePublishingasserts from inside the publishAnswer— the race is reproduced, not waited for.
Blocking
opendj-server-legacy/src/main/java/org/opends/server/replication/service/DSRSShutdownSync.java:115-117, :142-144 — opendj-server-legacy/src/main/java/org/opends/server/replication/plugin/PendingChanges.java:214-224
issue (blocking): A withdrawal empties a slot that announce() has already re-used, so the sent predecessor loses its wait.
replicaOfflineMsgSent is a put(): it replaces the replica's pending entry. On one domain, within the grace period:
disableService()→broker.stop()→ CSN1 announced and published; the collocated RS still has it queued to the peer RS behind a backlog (the case the forward guard's comment at:205-210names).enableService()→broker.start()— the connect fails silently (connectionError,ReplicationBroker.java:850-853) orconnectRequiresRecoveryis raised (LDAPReplicationDomain.java:5356-5363).- Second
broker.stop()→announce(CSN2)replaces CSN1's entry →publish()returns false →withdraw(CSN2)findspending.csn.equals(CSN2)and removes the slot.
awaitReplicaOfflineMsgsForwarded() now waits for nothing and CSN1 is never forwarded before the RS goes down — #919's guarantee is gone for the RS downtime. At BASE a refused CSN2 was never announced, so CSN1's wait survived. Producers: restartService() (back-to-back, from readAssuredConfig / readFractionalConfig), the total-update disable() / enable(), followed by shutdown() or another config change.
Suggested shape — a withdrawal puts back what the announcement displaced:
// PendingOfflineMsg
/** The announcement this one displaced and which is still owed its forward; null when there was none. */
private final PendingOfflineMsg displaced;
// replicaOfflineMsgSent
replicaOfflineMsgs
.computeIfAbsent(baseDN, dn -> new ConcurrentHashMap<>())
.compute(offlineCSN.getServerId(),
(id, displaced) -> new PendingOfflineMsg(offlineCSN, System.nanoTime(), displaced));
// replicaOfflineMsgNotSent
if (pending != null && pending.csn.equals(offlineCSN))
{
if (pending.displaced != null)
{
msgs.replace(serverId, pending, pending.displaced);
}
else
{
msgs.remove(serverId, pending);
}
}And the case for the reachable order (DSRSShutdownSyncTest):
/** The shutdown's message went out; the re-enable's was refused: the first one is still owed its forward. */
@Test
public void theWithdrawalOfALaterMessageGivesTheEarlierOneItsWaitBack() throws Exception
{
final DSRSShutdownSync shutdownSync = new DSRSShutdownSync(LONG_GRACE_PERIOD);
final CSN sentByTheShutdown = newCSN(SERVER_ID, 1);
final CSN refusedByTheBroker = newCSN(SERVER_ID, 2);
shutdownSync.replicaOfflineMsgSent(baseDN1, sentByTheShutdown);
shutdownSync.replicaOfflineMsgSent(baseDN1, refusedByTheBroker);
shutdownSync.replicaOfflineMsgNotSent(baseDN1, refusedByTheBroker);
assertThat(shutdownSync.canShutdown(baseDN1)).isFalse();
shutdownSync.replicaOfflineMsgForwarded(baseDN1, sentByTheShutdown, RS_ID);
assertThat(shutdownSync.canShutdown(baseDN1)).isTrue();
}opendj-server-legacy/src/test/java/org/opends/server/replication/plugin/PendingChangesTest.java:90-101, :231-242
issue (blocking): No case pins that the announcement of a published message stands — "withdraw unconditionally" is green 33/33.
Measured: with withdraw(offlineCSN) run on both arms of if (domain.publish(msg)), PendingChangesTest + DSRSShutdownSyncTest pass 33/33. theReplicaOfflineMsgIsAnnouncedBeforeItIsPublished forwards from inside the publish and then asserts only canShutdown == true; the refusal cases end on true as well. The only assertFalse(canShutdown) is inside refuseWhilePublishing (:252), so "announce deleted" dies once and "withdraw always" never. That mutant undoes #919 entirely and passes CI.
/** The announcement of a message the broker took stands until a peer forwards it. */
@Test
public void theAnnouncementOfAPublishedMessageStandsUntilItIsForwarded() throws Exception
{
final DSRSShutdownSync shutdownSync = new DSRSShutdownSync();
final PendingChanges pendingChanges = newPendingChanges(domainWhichPublishes(true), shutdownSync);
pendingChanges.putReplicaOfflineMsg();
assertFalse(shutdownSync.canShutdown(baseDN),
"the message went out and nobody has forwarded it yet, so the shutdown must wait for it");
}And in forwardWhilePublishing, before the forward — then case 1 pins its own name:
if (msg instanceof ReplicaOfflineMsg)
{
assertFalse(shutdownSync.canShutdown(baseDN), "the message must be announced before it is published");
shutdownSync.replicaOfflineMsgForwarded(baseDN, msg.getCSN(), RS_ID);
}Non-blocking
opendj-server-legacy/src/main/java/org/opends/server/replication/plugin/LDAPReplicationDomain.java:806-820
suggestion (non-blocking): The production announcer is exercised by no test — swapping replicaOfflineMsgSent and replicaOfflineMsgNotSent here survives the suite.
Every test builds its own ReplicaOfflineAnnouncer (PendingChangesTest:264-275) or announces by hand. A package-visible ShutdownSyncAnnouncer(DSRSShutdownSync, DN) in place of the anonymous class, plus one case — announce → canShutdown false, withdraw → true — pins the edge.
opendj-server-legacy/src/test/java/org/opends/server/replication/service/DSRSShutdownSyncTest.java:474-513
suggestion (non-blocking): theWaitEndsWhenTheMessageIsWithdrawn pins the wake-up only by elapsed < LONG_GRACE_PERIOD (60 s); a withdrawal that notifies without emptying the slot passes it.
assertThat(shutdownSync.canShutdown(baseDN1)).as("the withdrawn message holds nothing back").isTrue();opendj-server-legacy/src/test/java/org/opends/server/replication/service/DSRSShutdownSyncTest.java:106-122 — opendj-server-legacy/src/main/java/org/opends/server/replication/service/DSRSShutdownSync.java:128
todo (non-blocking): Both texts describe an interleaving that cannot happen: announce() and withdraw() run back to back under pushCommittedChanges()'s monitor (PendingChanges.java:175), one announcer per domain, one domain per baseDN per JVM — no "other thread of the domain" announces in between. The reachable second announcement is the one in the blocking issue above; theWithdrawalOfAnEarlierMessageLeavesANewerOneAlone pins the reverse order. Drop "another thread" from the javadoc and "a newer one made in the meantime" from :128, and say what the case does pin: a stale withdrawal is ignored.
opendj-server-legacy/src/main/java/org/opends/server/replication/service/DSRSShutdownSync.java:49
todo (non-blocking): "counted from the moment the message was sent" — with this PR the clock starts at the announcement, before the publish; :393 already says "announced".
PR description
suggestion (non-blocking): "what is announced is what really went out, and nothing else" is stronger than what the broker can report: Session.publish() returns silently for a pre-V8 peer (getBytes() == null, #1014) and after closeInitiated, and publish() reports true. Pre-existing (#976), only noting — "what the broker reports as written" is the claim that holds.
…published, not after it may have been forwarded The announcement the shutdown of a collocated replication server waits on was recorded after PendingChanges.putReplicaOfflineMsg() had already put the message on the wire. A forward which won that race found nothing to clear, and the announcement which followed it was one nothing would ever remove: ReplicationServer.shutdown() then spent the whole REPLICA_OFFLINE_GRACE_PERIOD waiting for the forward of a message the topology already had. The announcement now sits where the message is published - the ReplicaOfflineMsg branch of pushCommittedChanges() - so it is in place before session.publish() is reached and the forward cannot precede it. It goes through ShutdownSyncAnnouncer, the announcer of one domain and one DSRSShutdownSync, which the domain hands its PendingChanges. Announcing at the publish site also makes it follow the publication rather than the queueing, which leaves the guard OpenIdentityPlatform#918 put around the announcement nothing to do: a message a change in flight holds back is not published, and is therefore not announced either. The trace which reports such a message stays. The broker may still refuse the message once it is announced - no usable session, a recovery pending, or stopped in between - which OpenIdentityPlatform#949 made domain.publish() report. Such an announcement is one nobody will ever forward, so it is withdrawn through the new DSRSShutdownSync.replicaOfflineMsgNotSent(), which takes back only the entry carrying that CSN and wakes the shutdown up as a forward does: what stays announced is what the broker reports as written. A withdrawal gives back what the announcement displaced. A replica announces itself offline on every disableService(), and each announcement replaces the entry of the replica, so the refused message of a disable which follows a failed re-enable within the grace period had taken the place of the message the earlier disable did send - and withdrawing it emptied the slot that message was still waiting in. PendingOfflineMsg now keeps the announcement it displaced, and the withdrawal puts it back. New PendingChangesTest cases, all through the production announcer: the forward reported from within publish() finds the announcement in place and clears it, the announcement of a message the broker took stands until a peer forwards it, the announcement of a message the broker refused is withdrawn, and a message held back by a change in flight is never announced - neither while it waits, nor when the change which held it back completes and the message is given up on. DSRSShutdownSyncTest covers the withdrawal: it ends the wait and leaves nothing behind, it wakes a waiting shutdown up, it leaves a newer announcement of the same replica alone, and it gives an earlier one its wait back.
4aeb3b3 to
fba06c0
Compare
|
Round 1 addressed in Blocking 1 - a withdrawal emptied a slot One residue is named in the javadoc rather than handled: while CSN2 stands in CSN1's place, Blocking 2 - nothing pinned that the announcement of a published message stands. Confirmed
Production announcer untested.
"Another thread" / "a newer one made in the meantime". Both reworded. The test's javadoc now
"What really went out, and nothing else". The sentence was in the Tests, on the rebased head, class per JVM: |
Fixes #950
The bug
LDAPReplicationDomain.publishReplicaOfflineMsg()recorded the announcement afterpendingChanges.putReplicaOfflineMsg()returned, and that call has already put the message onthe wire:
pushCommittedChanges()reachesdomain.publish(msg)->ReplicationBroker.publish()->
session.publish(msg)before it comes back.A collocated replication server which forwards the message in that window calls
DSRSShutdownSync.replicaOfflineMsgForwarded()from itsServerWriter, which finds no entry forthe replica and does nothing but notify the monitor.
replicaOfflineMsgSent()then installs aPendingOfflineMsgwhich nothing will ever remove - the forward it was waiting for has alreadyhappened.
Since #919 that record is the condition of a blocking wait:
ReplicationServer.shutdown()callsawaitReplicaOfflineMsgsForwarded()and, with a peer RS connected, spends the wholeREPLICA_OFFLINE_GRACE_PERIODon a message which is on the wire and forwarded. Nothing is lost -the topology has the announcement - it is a bounded delay of the shutdown. Before #919 the stale
record was harmless, and the ordering it depends on has been there since OPENDJ-1453.
#946 has since narrowed which messages are announced - only those which really were published -
but left the ordering alone: the announcement of a published message still follows its publish.
The window is narrow: between the return of
session.publish()and the next statement of thepublishing thread, the collocated RS has to read the socket, write the changelog, queue the
message on the peer handler and write it to the peer session. But the cost of losing the race is
precisely the delay the grace period exists to bound.
The change
The announcement moves to the point where the message is published - the
ReplicaOfflineMsgbranch of
PendingChanges.pushCommittedChanges()- through aReplicaOfflineAnnouncerthedomain hands to its
PendingChanges. It is therefore in place beforesession.publish()isreached, and the
ConcurrentHashMapit is written to gives the forwarding thread, which reads itonly after reading the socket, the visibility it needs. The forward can no longer precede it.
The announcer the domain hands over is
ShutdownSyncAnnouncer, a package-private class of onedomain and one
DSRSShutdownSync:announce()isreplicaOfflineMsgSent(),withdraw()isreplicaOfflineMsgNotSent(). It is a class rather than an anonymous one so thatPendingChangesTestbuilds its pending changes with the very announcer the domain uses, and aswap of the two calls dies there.
Announcing at the publish site, rather than before the whole
putReplicaOfflineMsg(), also meansthe announcement follows the publication instead of the queueing. A message which a change in
flight holds back (#918) is not announced at all: #946 gives up on such a message rather than
letting it out late, so there is no later publish to announce it at.
That leaves the
if (offlineCSN != null)guard #946 put around the announcement nothing to do,which is what its own description predicted: a message which is not published is not announced.
publishReplicaOfflineMsg()keeps only the trace #946 added, with the wording #976 gave it.One announcement does have to be withdrawn. Since #976
domain.publish()reports whether thebroker wrote the message, and it refuses one when it has no usable session, when a recovery is
pending, or when it is stopped in between - all after the announcement was made. Such an
announcement is one nobody will ever forward, so
pushCommittedChanges()takes it back throughthe announcer, and
DSRSShutdownSync.replicaOfflineMsgNotSent()withdraws only the entrycarrying that CSN, and wakes the shutdown up as a forward does. This is the shape #950 proposed,
and what "not fixed here: #949" of the earlier revision of this description was waiting for.
What stays announced is what the broker reports as written - not more than that:
Session.publish()returns without writing for a peer which cannot decode the message and oncethe session's close is initiated, and the broker reports both as published. That is #976's
contract, unchanged here.
A withdrawal must not take an earlier announcement with it. A replica announces itself offline
on every
disableService(), andreplicaOfflineMsgSent()replaces the entry of the replica, soa shutdown whose message went out, followed within the grace period by an
enableService()whose connect fails or raises
connectRequiresRecovery, and then by anotherdisableService(),announces a second message the broker refuses - and withdrawing that one used to empty the slot
the first one was still waiting in.
PendingOfflineMsgnow keeps the announcement it displaced,and the withdrawal puts it back: the earlier message, which did go out, keeps its wait. Whatever
is reported about the earlier message while the refused one stands in its place - a forward, a
peer going away - is not seen by it, and the shutdown then waits out what is left of the earlier
message's own grace period; that window is the one refused publish, and the wait it can cost is
bounded by a grace period which is already running.
A trade-off worth naming
The grace period is now counted from just before the publish instead of just after it. Normally
that is microseconds. With the send window closed the broker loops on
tryAcquire(500 ms), and aslow publish eats part of the 5 seconds before the message even leaves. The direction is the safe
one - the wait can only end earlier, never later - and
newShutdownDeadline()bounds the wholeshutdown independently.
Tests
PendingChangesTestdrives a realDSRSShutdownSyncthrough the productionShutdownSyncAnnouncer.The five cases #946 and #976 left there are kept as they were, and four are new:
theReplicaOfflineMsgIsAnnouncedBeforeItIsPublishedreports the forward from insidepublish(), which is the moment the message reaches the session, so the race is reproducedrather than waited for. It asserts from there that the announcement is already in place, and
afterwards that the forward cleared it.
theAnnouncementOfAPublishedMessageStandsUntilItIsForwardedpins that an announcement thebroker took is not withdrawn: with nobody having forwarded it, the shutdown must wait.
theAnnouncementOfAReplicaOfflineMsgTheBrokerRefusedIsWithdrawnchecks from insidepublish()that the announcement is already in place, refuses the message the way a brokerwith no session does, and asserts nothing holds the shutdown back afterwards - so it pins a
withdrawal, not an announcement which was never made.
theReplicaOfflineMsgHeldBackByAChangeInFlightIsNeverAnnouncedpins the other half: nothing isannounced while a change in flight holds the message back, and nothing is announced when that
change completes either - [#918] Record a ReplicaOfflineMsg as sent only when it really was published #946 gives up on such a message rather than letting it out late.
DSRSShutdownSyncTestgrows four cases for the withdrawal: it ends the wait and leaves nothingbehind, it wakes a waiting shutdown up, the withdrawal of an earlier announcement leaves a newer
one of the same replica alone, and the withdrawal of a later announcement gives the earlier one
its wait back - which a forward of the earlier message then ends.
Three mutants were run against the suite, each dying where its name says:
domain.publish()-theReplicaOfflineMsgIsAnnouncedBeforeItIsPublishedandtheAnnouncementOfAReplicaOfflineMsgTheBrokerRefusedIsWithdrawn, both on "the message must beannounced before it is published";
if (domain.publish(msg))-theAnnouncementOfAPublishedMessageStandsUntilItIsForwarded;replicaOfflineMsgSentandreplicaOfflineMsgNotSentswapped inShutdownSyncAnnouncer-three cases of
PendingChangesTest.theWithdrawalOfALaterMessageGivesTheEarlierOneItsWaitBackwas watched failing before thedisplaced announcement was kept: "the earlier message went out and nobody has forwarded it yet -
expected false but was true".
Overlaps
for the reason it predicted; its trace, and its giving up on the message which stayed queued,
stay. Its test cases are unchanged.
enable(), which is elsewhere inLDAPReplicationDomainthanthe announcement this branch moves, so the two merged with nothing to reconcile.
with
domain.publish()reporting a refusal, an announcement made before the publish hassomething to be withdrawn for.
pushCommittedChanges()keeps reporting the CSN of the messagethe broker accepted, so
putReplicaOfflineMsg()and the trace behave as [#949] Report a ReplicaOfflineMsg the broker refused as not sent #976 left them.put()records therecipients -
replicaOfflineMsgDispatched()is a no-op without it. This change makes thathold, leaving its
awaitedForwarders == nullfallback for the replica which picked a remotereplication server; the other case that fallback named, an announcement recorded after its
message was relayed, no longer exists, and its comment says so.
(
cebef54070). None of them touches the announcement, the announcer orDSRSShutdownSync;the merge was clean.