Skip to content

Commit

Permalink
Fix longest log verification during election
Browse files Browse the repository at this point in the history
* Updated the check to keep track of the "highest" VoteResponse received
  and perform checks against it.
* Expanded test to cover the three possibilities scenarios in the
  {A,B,C} view.

Closes #251.
  • Loading branch information
jabolina committed Feb 12, 2024
1 parent c14152c commit b87f5f3
Show file tree
Hide file tree
Showing 2 changed files with 69 additions and 22 deletions.
43 changes: 30 additions & 13 deletions src/org/jgroups/protocols/raft/election/BaseElection.java
Original file line number Diff line number Diff line change
Expand Up @@ -229,32 +229,49 @@ private void handleVoteResponse(Message msg, VoteResponse hdr) {

protected Address determineLeader() {
Address leader=null;
VoteResponse higher = null;
Map<Address,VoteResponse> results=votes.getResults();
for(Address mbr: view.getMembersRaw()) {
VoteResponse rsp=results.get(mbr);
if(rsp == null)
continue;
if(leader == null)
leader=mbr;
if(isHigher(rsp.last_log_term, rsp.last_log_index))
leader=mbr;
if (isHigher(higher, rsp)) {
leader = mbr;
higher = rsp;
}
}
return leader;
}

/**
* Returns true if last_term greater than my own term, false if smaller. If they're equal, returns true if
* last_index is > my own last index, false otherwise
* Compares the {@link VoteResponse}s in search of the highest one.
* <p>
* The verification follows the precedence:
* <ol>
* <li>Compare Raft terms;</li>
* <li>Compare the log last appended.</li>
* </ol>
* This verification ensures the decided leader has the longest log.
* </p>
*
* @param one: The base to check against.
* @param other: The candidate response to check.
* @return <code>true</code> if {@param other} is higher than {@param one}. <code>false</code>, otherwise.
*/
protected boolean isHigher(long last_term, long last_index) {
long my_last_index=raft.log().lastAppended();
LogEntry entry=raft.log().get(my_last_index);
long my_last_term=entry != null? entry.term() : 0;
if(last_term > my_last_term)
private boolean isHigher(VoteResponse one, VoteResponse other) {
if (one == null) return true;

// The candidate response has a higher Raft term.
if (one.last_log_term < other.last_log_term)
return true;
if(last_term < my_last_term)

// The candidate response has an outdated Raft term.
if (one.last_log_term > other.last_log_term)
return false;
return last_index > my_last_index;

// Both responses have the same term.
// Break ties utilizing the index of the last appended entry.
return one.last_log_index < other.last_log_index;
}

/**
Expand Down
48 changes: 39 additions & 9 deletions tests/junit-functional/org/jgroups/tests/ElectionsTest.java
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@
import java.util.ArrayList;
import java.util.Arrays;
import java.util.List;
import java.util.Map;

import org.testng.annotations.Test;

Expand Down Expand Up @@ -54,18 +55,46 @@ public void testSimpleElection(Class<?> ignore) {
/**
* B and C have longer logs than A: one of {B,C} must become coordinator, but *not* A
*/
public void testElectionWithLongLog(Class<?> ignore) {
public void testElectionWithLongLogTie(Class<?> ignore) {
testLongestLog(Map.of(b, new int[]{1, 1, 2}, c, new int[]{1, 1, 2}), b.getAddress(), c.getAddress());
}

/**
* B has the longest term in the view {A, B, C}.
* Node B has to become the leader.
*/
public void testElectionWithLongLogMiddle(Class<?> ignore) {
int[] termsB = new int[] { 1, 1, 2, 2 };
int[] termsC = new int[] { 1, 1, 2 };
testLongestLog(Map.of(b, termsB, c, termsC), b.getAddress());
}

/**
* C has the longest term in the view {A, B, C}.
* Node C has to become the leader.
*/
public void testElectionWithLongLogLast(Class<?> ignore) {
int[] termsB = new int[] { 1, 1, 2 };
int[] termsC = new int[] { 1, 1, 2, 2 };
testLongestLog(Map.of(b, termsB, c, termsC), c.getAddress());
}

private void testLongestLog(Map<JChannel, int[]> logs, Address ... possibleElected) {
// Let node A be elected the first leader.
assertLeader(10_000, a.getAddress(), a, b, c);

// Add the entries, creating longer logs.
setLog(b, 1, 1, 2);
setLog(c, 1, 1, 2);
for (Map.Entry<JChannel, int[]> entry : logs.entrySet()) {
setLog(entry.getKey(), entry.getValue());
}

// Assert that B and C have a longer log.
// Assert nodes have longer logs than A.
long aSize = logLastAppended(a);
assert aSize < logLastAppended(b) : "A log longer than B";
assert aSize < logLastAppended(c) : "A log longer than C";

for (JChannel ch : logs.keySet()) {
assertThat(aSize)
.withFailMessage(() -> String.format("Node A has a log longer than %s", ch.getAddress()))
.isLessThan(logLastAppended(ch));
}

JChannel coord = findCoord(a, b, c);
assertThat(coord).isNotNull();
Expand All @@ -82,9 +111,10 @@ public void testElectionWithLongLog(Class<?> ignore) {
el.startVotingThread();
waitUntilVotingThreadStops(5_000, 0, 1, 2);

System.out.printf("-- waiting for leader in %s", Arrays.toString(possibleElected));
Address leader = assertLeader(10_000, null, b, c);
assert leader.equals(b.getAddress()) || leader.equals(c.getAddress()) : dumpLeaderAndTerms();
assert !leader.equals(a.getAddress());
assertThat(possibleElected).contains(leader);
assertThat(leader).isNotEqualTo(a.getAddress());
}

protected static JChannel findCoord(JChannel... channels) {
Expand Down

0 comments on commit b87f5f3

Please sign in to comment.