Skip to content
This repository has been archived by the owner on Mar 4, 2024. It is now read-only.

Committing entries from previous terms #220

Closed
as58545652 opened this issue Jul 9, 2021 · 8 comments · Fixed by #336
Closed

Committing entries from previous terms #220

as58545652 opened this issue Jul 9, 2021 · 8 comments · Fixed by #336
Assignees
Labels
Bug Confirmed to be a bug

Comments

@as58545652
Copy link

as58545652 commented Jul 9, 2021

raft/src/replication.c

Lines 1539 to 1574 in 4c71b94

void replicationQuorum(struct raft *r, const raft_index index)
{
size_t votes = 0;
size_t i;
assert(r->state == RAFT_LEADER);
if (index <= r->commit_index) {
return;
}
/* TODO: fuzzy-test --seed 0x8db5fccc replication/entries/partitioned
* fails the assertion below. */
if (logTermOf(&r->log, index) == 0) {
return;
}
// assert(logTermOf(&r->log, index) > 0);
assert(logTermOf(&r->log, index) <= r->current_term);
for (i = 0; i < r->configuration.n; i++) {
struct raft_server *server = &r->configuration.servers[i];
if (server->role != RAFT_VOTER) {
continue;
}
if (r->leader_state.progress[i].match_index >= index) {
votes++;
}
}
if (votes > configurationVoterCount(&r->configuration) / 2) {
r->commit_index = index;
tracef("new commit index %llu", r->commit_index);
}
return;
}

From section 3.6.2:
Raft never commits log entries from previous terms by counting replicas. Only log entries from the leader’s current term are committed by counting replicas.

In the code, I have not found the implementation of the logic described above.

@MathieuBordere
Copy link
Contributor

Thanks for logging this issue, good catch!

The quorum logic should implement the part in Rules for Servers - Leaders at the bottom of figure 2 in raft extended

If there exists an N such that N > commitIndex, 
a majority of matchIndex[i] ≥ N, 
and log[N].term == currentTerm 
: set commitIndex = N.

This does not seem to be the case in our implementation.

@MathieuBordere MathieuBordere added the Bug Confirmed to be a bug label Jul 13, 2021
@MathieuBordere MathieuBordere self-assigned this Jul 13, 2021
@calvin2021y
Copy link

Any plan to fix this?

@MathieuBordere
Copy link
Contributor

Yes, it is still the plan to fix this.

@calvin2021y
Copy link

This bugs find almost a year, Is there a schedule plan for it.

@MathieuBordere
Copy link
Contributor

I have a branch where I'm implementing a fix as it has some unintended consequences in dqlite, however, unfortunately, things keep coming up in between to finish it.

@calvin2021y
Copy link

Thanks for the quick response, look forward to it.

If you can share your branch here, we can try it and maybe give some advice.

@MathieuBordere
Copy link
Contributor

https://github.com/MathieuBordere/raft/commits/quorum-bugfix it's behind the current master somewhat though.

@cole-miller
Copy link
Contributor

I'm in the process of picking up Mathieu's previous work on this, will have a draft PR up soon.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Bug Confirmed to be a bug
Projects
None yet
4 participants