Skip to content

Commit

Permalink
Remove dead ordered client election code (microsoft#22488)
Browse files Browse the repository at this point in the history 
[AB#15192](https://dev.azure.com/fluidframework/235294da-091d-4c29-84fc-cdfc3d90890b/_workitems/edit/15192)

Removes dead code from `OrderedClientElection`. 

Historical note: we had developed a feature that elected a new
summarizer client when the current summarizer client was failing to
summarize. When we shipped the feature and turned it on, we ran into
many buggy issues with multiple summarizers. Since then, we've turned
off the feature and have some dead code. This PR aims to remove that
dead code.
  • Loading branch information
@tyler-cai-microsoft
tyler-cai-microsoft authored Sep 27, 2024
1 parent 107be83 commit 5989ab2
Show file tree
Hide file tree
Showing 2 changed files with 0 additions and 177 deletions.
19 changes: 0 additions & 19 deletions packages/runtime/container-runtime/src/summary/orderedClientElection.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -270,8 +270,6 @@ export interface IOrderedClientElection extends IEventProvider<IOrderedClientEle
readonly electedParent: ITrackedClient | undefined;
/** Sequence number of most recent election. */
readonly electionSequenceNumber: number;
/** Marks the currently elected client as invalid, and elects the next eligible client. */
incrementElectedClient(sequenceNumber: number): void;
/** Resets the currently elected client back to the oldest eligible client. */
resetElectedClient(sequenceNumber: number): void;
/** Peeks at what the next elected client would be if incrementElectedClient were called. */
Expand Down Expand Up @@ -581,23 +579,6 @@ export class OrderedClientElection
return this.orderedClientCollection.getAllClients().filter(this.isEligibleFn);
}

/**
* Advance election to the next-oldest client. This is called if the current parent is leaving the quorum,
* or if the current summarizer is not responsive and we want to stop it and spawn a new one.
*/
public incrementElectedClient(sequenceNumber: number): void {
const nextClient =
this.findFirstEligibleParent(this._electedParent?.youngerClient) ??
this.findFirstEligibleParent(this.orderedClientCollection.oldestClient);
if (this._electedClient === undefined || this._electedClient === this._electedParent) {
this.tryElectingClient(nextClient, sequenceNumber, "IncrementElectedClient");
} else {
// The _electedClient is a summarizer and should not be replaced until it leaves the quorum.
// Changing the _electedParent will stop the summarizer.
this.tryElectingParent(nextClient, sequenceNumber, "IncrementElectedClient");
}
}

/**
* (Re-)start election with the oldest client in the quorum. This is called if we need to summarize
* and no client has been elected.
Expand Down
158 changes: 0 additions & 158 deletions packages/runtime/container-runtime/src/test/summary/orderedClientElection.spec.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -139,12 +139,6 @@ describe("Ordered Client Collection", () => {
election.on("election", () => electionEventCount++);
return election;
}
function incrementElectedClient(sequenceNumber = currentSequenceNumber) {
if (sequenceNumber > currentSequenceNumber) {
currentSequenceNumber = sequenceNumber;
}
election.incrementElectedClient(sequenceNumber);
}
function resetElectedClient(sequenceNumber = currentSequenceNumber) {
if (sequenceNumber > currentSequenceNumber) {
currentSequenceNumber = sequenceNumber;
Expand Down Expand Up @@ -445,23 +439,6 @@ describe("Ordered Client Collection", () => {
assertOrderedEligibleClientIds("a", "b");
});

it("Should remove elected eligible client from end", () => {
createOrderedClientElection([
["a", 1, true],
["b", 2, true],
["s", 5, false],
["c", 9, true],
]);
incrementElectedClient(12); // elect b
incrementElectedClient(19); // elect c
assertElectionState(4, 3, "c", 19);
assertEvents(2);
removeClient("c", 5);
assertElectionState(3, 2, "a", 24);
assertEvents(3);
assertOrderedEligibleClientIds("a", "b");
});

it("Should remove other eligible client from middle", () => {
createOrderedClientElection([
["a", 1, true],
Expand All @@ -475,22 +452,6 @@ describe("Ordered Client Collection", () => {
assertOrderedEligibleClientIds("a", "c");
});

it("Should remove elected eligible client from middle", () => {
createOrderedClientElection([
["a", 1, true],
["b", 2, true],
["s", 5, false],
["c", 9, true],
]);
incrementElectedClient(12); // elect b
assertElectionState(4, 3, "b", 12);
assertEvents(1);
removeClient("b", 5);
assertElectionState(3, 2, "c", 17);
assertEvents(2);
assertOrderedEligibleClientIds("a", "c");
});

it("Should remove elected eligible client from front", () => {
createOrderedClientElection([
["a", 1, true],
Expand All @@ -504,22 +465,6 @@ describe("Ordered Client Collection", () => {
assertOrderedEligibleClientIds("b", "c");
});

it("Should remove other eligible client from front", () => {
createOrderedClientElection([
["a", 1, true],
["b", 2, true],
["s", 5, false],
["c", 9, true],
]);
incrementElectedClient(12); // elect b
assertElectionState(4, 3, "b", 12);
assertEvents(1);
removeClient("a", 5);
assertElectionState(3, 2, "b", 12);
assertEvents(1);
assertOrderedEligibleClientIds("b", "c");
});

it("Should elect next client when ineligible client is elected, then elected client is removed", () => {
createOrderedClientElection(
[
Expand All @@ -538,110 +483,7 @@ describe("Ordered Client Collection", () => {
});
});

describe("Increment elected client", () => {
it("Should do nothing in empty quorum", () => {
createOrderedClientElection();
incrementElectedClient();
assertElectionState(0, 0, undefined, 0);
assertEvents(0);
});

it("Should go to next client from first", () => {
createOrderedClientElection([
["a", 1, true],
["b", 2, true],
["s", 5, false],
["c", 9, true],
]);
incrementElectedClient(12);
assertElectionState(4, 3, "b", 12);
assertEvents(1);
});

it("Should go to next client from middle", () => {
createOrderedClientElection([
["a", 1, true],
["b", 2, true],
["s", 5, false],
["c", 9, true],
]);
incrementElectedClient(12);
incrementElectedClient(16);
assertElectionState(4, 3, "c", 16);
assertEvents(2);
});

it("Should increment to new nodes", () => {
createOrderedClientElection([
["a", 1, true],
["b", 2, true],
["s", 5, false],
["c", 9, true],
]);
incrementElectedClient(16);
incrementElectedClient(27); // no-op
addClient("d", 100);
incrementElectedClient(100);
addClient("e", 101);
assertElectionState(6, 5, "d", 100);
incrementElectedClient(111);
assertElectionState(6, 5, "e", 111);
addClient("f", 200);
incrementElectedClient(205);
assertElectionState(7, 6, "f", 205);
incrementElectedClient(221);
assertElectionState(7, 6, "a", 221);
addClient("g", 229);
assertElectionState(8, 7, "a", 221);
});

it("Should increment when ineligible client is elected", () => {
createOrderedClientElection(
[
["a", 1, true],
["s", 2, false],
["b", 5, true],
["c", 9, true],
],
{ electedClientId: "s", electedParentId: "s", electionSequenceNumber: 4321 },
);
assertElectionState(4, 3, "b", 4321);
incrementElectedClient(7777);
assertElectionState(4, 3, "c", 7777);
assertEvents(1);
});
});

describe("Reset elected client", () => {
it("Should reset to first when not first", () => {
createOrderedClientElection([
["a", 1, true],
["b", 2, true],
["s", 5, false],
["c", 9, true],
]);
incrementElectedClient(12);
incrementElectedClient(15);
resetElectedClient(19);
assertElectionState(4, 3, "a", 19);
assertEvents(3);
});

it("Should reset to first when undefined at end", () => {
createOrderedClientElection([
["a", 1, true],
["b", 2, true],
["s", 5, false],
["c", 9, true],
]);
incrementElectedClient(12);
incrementElectedClient(15);
incrementElectedClient(19);
resetElectedClient(31); // no-op
assertElectionState(4, 3, "a", 19);
assertEvents(3);
});

it("Should reset to first when ineligible client is elected", () => {
createOrderedClientElection(
[
Expand Down

0 comments on commit 5989ab2

Please sign in to comment.