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

Fix last_applied failing assertion #423

Merged
merged 2 commits into from
Jun 2, 2023
Merged

Fix last_applied failing assertion #423

merged 2 commits into from
Jun 2, 2023

Conversation

freeekanayaka
Copy link
Contributor

This fixes #355, see the reproducing test and commit messages for details.

Add a failing test that reproduces the situation that triggered the assertion
failure described in #355.

Signed-off-by: Free Ekanayaka <free@ekanayaka.io>
A leader might have its last_stored field lagging behind its commit_index,
after it committed an index that it did not yet persist, while enough followers
for forming a majority did.

We don't wont to update this leader's commit_index until last_stored has caught
up.

Signed-off-by: Free Ekanayaka <free@ekanayaka.io>
@codecov
Copy link

codecov bot commented Jun 1, 2023

Codecov Report

Merging #423 (d144504) into master (7cfccd3) will decrease coverage by 0.01%.
The diff coverage is 66.66%.

@@            Coverage Diff             @@
##           master     #423      +/-   ##
==========================================
- Coverage   76.64%   76.64%   -0.01%     
==========================================
  Files          50       50              
  Lines        9571     9573       +2     
  Branches     2439     2441       +2     
==========================================
+ Hits         7336     7337       +1     
  Misses       1162     1162              
- Partials     1073     1074       +1     
Impacted Files Coverage Δ
src/replication.c 69.15% <66.66%> (-0.05%) ⬇️

@freeekanayaka
Copy link
Contributor Author

Note that this fix is probably a bit too brute force, as I believe there's no reason to not allow followers to have their last_stored index lag behind their commit_index, pretty much like leaders do already: as soon as they receive from a leader a commit_index that they already have appended to their in-memory log, it's safe for the follower to increase its commit_index and apply it to the FSM, even if that entry is still being persisted. That entry could also then fail to be persisted and that would be fine too, because the FSM has applied a committed entry, albeit not yet persisted locally (again, exactly like leaders already do). We acknowledge to leaders only persisted entries, which is what counts for commitment, and that's unrelated to what we've applied to the FSM.

However I'd keep implementing the above as a possible follow-up improvement.

@cole-miller cole-miller self-requested a review June 1, 2023 20:43
@MathieuBordere MathieuBordere merged commit c133337 into canonical:master Jun 2, 2023
16 of 19 checks passed
@freeekanayaka freeekanayaka deleted the fix-last-applied-assertion branch June 2, 2023 13:01
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Assertion `r->last_applied <= r->commit_index' failed.
3 participants